-
-
Notifications
You must be signed in to change notification settings - Fork 15
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
Update Snapshot Action: fix handling of the browser
option
#183
base: main
Are you sure you want to change the base?
Conversation
Need to double check how |
I think this is an improvement as-is. This fixes #183. Should we merge? |
I think it's still a draft because it wasn't clear how Playwright would handle the empty ( Not sure I'll have the bandwith to get back to it in the coming days, so if you would like to pick up this PR then feel free, thanks! |
I am not sure if we ever need to test all browsers in a single run of the action. If we want to test on multiple browsers we would likely split these into multiple runs anyways so that it is easier to rerun flaky jobs. |
Co-authored-by: Michał Krassowski <[email protected]>
Thanks @krassowski for the review.
Yes makes sense to use a test matrix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jtpio @krassowski I guess this is ok to be merged, isn't it?
Yes it should be to good to go. |
The
browser
input did not seem to be used.browser
to theupdate_script