Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"New Context" and "New Persistent Context" keywords should always return a Path-object with full video path in recordVideo dictionary #2215

Closed
ssallmen opened this issue Jul 27, 2022 · 11 comments · Fixed by #2524
Assignees
Labels
bug Something isn't working priority: high
Milestone

Comments

@ssallmen
Copy link

ssallmen commented Jul 27, 2022

Is your feature request related to a problem? Please describe.
At the moment it depends on whether the directory given in the dictionary as recordVideo argument already exists or not, whether the New Context keyword mutates the dir-key value to a Path object, or does it keep it as a string. That makes things inconsistent.
Even more inconsistent is that New Persistent Context keyword never mutates the dir-key value of the recordVideo argument dictionary but keeps it untouched, even when user has given a relative path. New Context keyword will mutate the dir-keys value with the full path as Path object if the path given by user did not exist, or if the given path was relative.
There is also another bug in New Permanent Context keyword related to the video recording which I reported separately in #2214

Describe the solution you'd like
Both New Permanent Context and New Context should always "return", independent whether the path was existing or not or if it was relative or absolute, the full path either as a Path-object or as a string as the value of the dir-key in dictionary given as recordVideo argument.

Additional context
I have code already prepared for the change (always returning Path-object as that's what we discussed with @aaltat). Maybe some tests need to be added to make that a PR.

@Snooz82
Copy link
Member

Snooz82 commented Jan 7, 2023

New Page does return the Path to the video file of that particular page.
New Context without a page has no video, so it does not help to return that.

New Persistent Context will return a tuple of browserId , contextID and NewPageDetails with 15.0.0

NewPageDetails is a dictionary that contains the keys page_id and video_path

Snooz82 added a commit that referenced this issue Jan 7, 2023
Signed-off-by: René <snooz@posteo.de>
@ssallmen
Copy link
Author

ssallmen commented Jan 7, 2023

@Snooz82, nice to see that you have done good enhancement for the New Permanent Context keyword but I am not sure if you fully understood the issue in New Context keyword, as it was a bit complex to explain. By "return" I did not mean the return value of the keyword but the mutation of the recordVideo dictionary values, as I tried to explain in the beginning of the ticket description:

At the moment it depends on whether the directory given in the dictionary as recordVideo argument already exists or not, whether the New Context keyword mutates the dir-key value to a Path object, or does it keep it as a string.

I don't know if that issue has been fixed already as it has been a while when I created the ticket, and many releases have been made, but my opinion is that the recordVideo.dir should be always handled the same way (i.e. mutated to a Path or kept as a string) independent on whether the path given as the value of recordVideo.dir already existed or not. It is a bit different bug than what is now fixed with that PR, and that PR should not close this ticket.

@Snooz82
Copy link
Member

Snooz82 commented Jan 8, 2023

You are correct that is not yet solved.

I did not yet get it completely.
We removed the videoPath argument now.

So the only way is now the recordVideo dictionary.

the rules of the paths have changed a bit and are now:

  1. if no path is given, videos are stored under ${OUTPUT_DIR}/browser/video
  2. If a relative path is given, it is relative to the path in 1.
  3. If an absolute Path is given, it is stored there.

So are you talking about the resulting paths in Browser Catalog? Get Browser Catalog

@Snooz82
Copy link
Member

Snooz82 commented Jan 8, 2023

Can you describe some reproducible cases and the effect to the users?

where is the difference between a path object given or a string?

@aaltat
Copy link
Member

aaltat commented Jan 8, 2023

@allcontributors please add @ssallmen for bugs

@allcontributors
Copy link
Contributor

@aaltat

@ssallmen already contributed before to bug

@Snooz82
Copy link
Member

Snooz82 commented Jan 8, 2023

@ssallmen

I have now implemented it so, that the path will always be a resolved and absolute Path.

def _set_video_path(self, params: dict) -> dict:
        record_video = params.get("recordVideo", {})
        video_path = record_video.get("dir")
        if not record_video:
            return params
        if video_path is None:
            vid_path = self.video_output
        elif Path(video_path).is_absolute():
            vid_path = params["recordVideo"]["dir"]
        else:
            vid_path = self.video_output / video_path
        params["recordVideo"]["dir"] = Path(vid_path).resolve().absolute()
        return params

@ssallmen-pro
Copy link
Contributor

ssallmen-pro commented Jan 8, 2023

@Snooz82, this comment in another bug report explains the best how it used to work earlier, and what were the impications for the user: #2206 (comment)
I have now created couple of test suites and ran them using the latest release (14.4.1) of Brower library available in PyPi, and found out that the behaviour has changed at some point so that the recordVideo.dir will always be string, independent on whether the path is absolute, relative, existing or non-existing. And that holds now true with both New Context as well as New Persistent Context. That is very good thing.
I am attaching here couple of Robot test suites I just created for a sample, and a Python library which helped me with validating the types as with Robot handling variable types is a bit challenging. I was not able to run the same tests using teh version I used at that point (most probably 13.2.0) as for some reason the grpcio version (1.47.0) required by that version of Browser library did not install on the laptop I have at hand now. But that does not matter anymore. This issues seems to be fixed now even with the current released version.

@Snooz82 Snooz82 self-assigned this Jan 8, 2023
@Snooz82 Snooz82 added this to the v15.0.0 milestone Jan 8, 2023
@Snooz82
Copy link
Member

Snooz82 commented Jan 8, 2023

@ssallmen @ssallmen-pro

Ok now i understand the issue.
Yes. ok.

I used your tests and all pass.
The original dictionary is not changed/mutated anymore.

If it contains a string, it stays a string, if it contains a Path object it stays a Path object.

@Snooz82
Copy link
Member

Snooz82 commented Jan 8, 2023

@allcontributors please add @ssallmen for tests

@allcontributors
Copy link
Contributor

@Snooz82

I've put up a pull request to add @ssallmen! 🎉

@Snooz82 Snooz82 added bug Something isn't working priority: high labels Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority: high
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants