Convert typed dict earlier in new_context

[ssallmen]

To avoid possible argument mutation for arguments that have pointers, like dictionaries

[aaltat]

To protect accidentally changing that code again, it would be good idea to modify example this test to check the dictionary https://github.com/MarketSquare/robotframework-browser/blob/main/atest/test/01_Browser_Management/video.robot

At least in one test.

[ssallmen]

I was just looking into it also. Will try to add or modify two tests for this behavior to a test in that suite. Two because the original behavior was different depending on whether the video path existed beforehand or not.

[ssallmen]

Is the New Context actually expected to return the full video record path in the dictionary passed to it using recordVideo argument? That came into my mind when I was about to modify test case Create Video With Relative Path in https://github.com/MarketSquare/robotframework-browser/blob/main/atest/test/01_Browser_Management/video.robot. If that should be the case, then I think better way would be to return that path as a string in the passed recordVideo dictionary, and not as a WindowsPath object as it is now. And it should be documented that the function will return the full path in there.

[ssallmen]

I will anyhow add the tests here, and you can then evaluate if the expectations in the tests are ok.

[aaltat]

1. Full path
   This I agree and it should be backwards icompatible

2. As string
   This I am reluctant to change, because it's backwards incompatible. Also Path objects are handy in many cases. Is Remove Directory keyword only reason for you to need it as string?

[aaltat]

F☆☆☆☆ did accidentally press the update branch button, instead of approving the CI runs. Those are so close in mobile, sorry. Please force push your local branch to PR.

[ssallmen]

Not only because of Remove Directory keyword but basically any keyword that is expecting to get a path as string. I don't know any other Robot Framework library than possibly Browser that would accept paths as a WindowsPath object.

[ssallmen]

With "force push your local branch to PR" do you mean git push [email protected]:ssallmen/robotframework-browser.git --force or something else?

[ssallmen]

Ok, when I read carefully your comment on what you accidentally did I think I know what you meant. I will do that.

[aaltat]

With "force push your local branch to PR" do you mean git push [email protected]:ssallmen/robotframework-browser.git --force or something else?

Yes that one please.

[ssallmen]

Force push done to get back to the previous state of the main branch

[ssallmen]

About the backwards incompatibility if returning strings. In the current version of Browser library, the New Context keyword returns the recordVideo.dir as string if the path existed already, but it returns it as a WindowsPath object if the path did not exist earlier. And, New Persistent Context will not mutate the given recordVideo dictionary at all. So, which one should be the one it returns, WindowsPath or string? I think it should be the same always, and it should be documented, or otherwise recordVideo dictionary should not be mutated at all, which I am now doing with this PR.

[aaltat]

Path objects are the future and more underlying native Python methods gain support for Path objects. Example subprocess.Popen got Path object support in 3.8 https://docs.python.org/3.8/library/subprocess.html#popen-constructor Example Process library works with Path objects when used with Python 3.8 or greater, but not with Python 3.7 or lower.

Returning two different types is not good, Path object would be better.

Also RF OperatingSystem library should start supporting Path objects too, but that is separate issue.

[ssallmen]

Ok, should we then change the implementation of new_context and new_persistent_context so that it will always return a Path-object? If you think so, I could make another PR for that and we can then close this PR. I think I have the code already for that, just need to add and modify some tests too.

[ssallmen]

At the same time I found another bug in new_persistent_context. Calling new_page after new_persistent_context will currently fail if we enable video recording by giving recordVideo argument for new_persistent_context. The failure is because the context uuid with the video size is not stored to context_cache at all and _embed_video called by new_page is expecting the video's context uuid to be in the context_cache to get the size of the video. May I fix that with the same PR?

[ssallmen]

I have now created a separate bug report of the "New Persistent Context" keyword: #2214
And, I have also written a separate issue on the recordVideo dir-key value: #2215
It is now up to you to decide what is the best way to fix these inconsistencies and/or bugs, and possibly close this PR if you see that better.

[mkorpela]

@ssallmen thanks for the contribution!

[mkorpela]

Hmmm tests failing :/