Skip to content

Conversation

@gsnedders
Copy link
Member

This ensures test_merge_platform_name and test_merge_browserName both
work even when a platformName and browserName capability is provided
in the default capabilities. And while we're fixing that, also fix the
case that pageLoadStrategy is provided.

Copy link
Contributor

@karlcow karlcow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not fully sure about the del statement there.

Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's ensure that each test only verifies the specific behavior indicated by its name.

@gsnedders
Copy link
Member Author

Let's ensure that each test only verifies the specific behavior indicated by its name.

I presume these use pageLoadStrategy to be able to ensure that firstMatch is behaving correctly — that browserName/platformName in firstMatch isn't just being totally ignored, as otherwise they would pass. This seems to go all the way back to when @jgraham originally wrote the tests in ec5cd8c.

We certainly can remove pageLoadStrategy, but then it becomes indistinguishable which firstMatch is being applied (if any!), and when the test is supposedly testing the merging, that seems important?

@gsnedders gsnedders force-pushed the test_merge_platform_name branch from 1b0c69b to a9c2bb6 Compare May 20, 2025 23:27
Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes. Thank you for the explanation. So lets apply the following suggestion to the relevant tests to make it clearer.

This ensures test_merge_platform_name and test_merge_browserName both
work even when a platformName and browserName capability is provided
in the default capabilities. And while we're fixing that, also fix the
case that pageLoadStrategy is provided.
@gsnedders gsnedders force-pushed the test_merge_platform_name branch from a9c2bb6 to 672067b Compare May 21, 2025 18:20
@gsnedders gsnedders enabled auto-merge (rebase) May 21, 2025 18:21
@gsnedders gsnedders merged commit 70ffd08 into web-platform-tests:master May 21, 2025
17 checks passed
@gsnedders gsnedders deleted the test_merge_platform_name branch May 21, 2025 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants