Track the OS for browser platforms #2449
Conversation
We currently do not allow making configuration using tools like
`OnPlatform` that is specific to an OS and browser combination. So if
there is a situation where tests don't work on firefox on windows, but
work everywhere else, we cannot express an intent to skip just the
failing tests.
In a usage like `OnPlatform({'windows && firefox'})` the selector will
never be true because `firefox` and `windows` are mutually exclusive.
It does allow cases like `'windows || firefox'` which matches windows VM
tests and firefox everywhere.
Changing this means a change to how existing selectors are evaluated. CI
may be impacted if a package is configuring a skip for an OS expecting
only the VM tests to be skipped on that OS with the browser tests still
running.
Use the new capability to skip a test that is failing on windows firefox
browser.
PR HealthChangelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. |
| @@ -38,9 +38,6 @@ final class SuitePlatform { | |||
| this.os = OperatingSystem.none, | |||
| this.inGoogle = false}) | |||
| : compiler = compiler ?? runtime.defaultCompiler { | |||
| if (runtime.isBrowser && os != OperatingSystem.none) { | |||
There was a problem hiding this comment.
It seems like this was intentional for some reason so it might be worth dragging up the original PR?
There was a problem hiding this comment.
The ArgumentError was added in #778
From that commit it doesn't look like the browser/OS restriction was present prior. The restriction that did exist is that an OS couldn't be non-null if a platform was null.
decc01d#diff-503be8017a1b23c25555e53d9976a7a67da81d39a671a51bd5a6a2eddbf18197L53-L56
I don't see any code in the before on that diff which conditionally added an OS or did anything different on browsers with regards to the OS. The code for resolving the platform selectors looks the same.
|
I'm iffy on whether I want to land this, I'm not entirely sure of the risks of changing the value of OS selectors for all configuration of browser tests. The main concern is that packages could be intentionally using the current meaning to skip all VM tests on some OS but run all browser tests on that OS. There is recourse - you can change |
|
I do think its probably a good change... you don't expect |
We currently do not allow making configuration using tools like
OnPlatformthat is specific to an OS and browser combination. So ifthere is a situation where tests don't work on firefox on windows, but
work everywhere else, we cannot express an intent to skip just the
failing tests.
In a usage like
OnPlatform({'windows && firefox'})the selector willnever be true because
firefoxandwindowsare mutually exclusive.It does allow cases like
'windows || firefox'which matches windows VMtests and firefox everywhere.
Changing this means a change to how existing selectors are evaluated. CI
may be impacted if a package is configuring a skip for an OS expecting
only the VM tests to be skipped on that OS with the browser tests still
running.
Use the new capability to skip a test that is failing on windows firefox
browser.