Enable support for running the test harness in Safari. #25499
Conversation
juj
changed the title
test/common.py
Outdated
| # by the delta before->after. | ||
| cls.browser_procs = list(set(procs_after).difference(set(procs_before))) | ||
| if len(cls.browser_procs) == 0: | ||
| logger.warning('Could not detect the launched browser subprocesses. The test harness may not be able to close browser windows if a test hangs, and at harness exit.') |
There was a problem hiding this comment.
Can this be an error instead?
There was a problem hiding this comment.
I can make it an error, but it is only so when running with EMTEST_BROWSER_AUTO_CONFIG enabled. (I think btw disabling EMTEST_BROWSER_AUTO_CONFIG might currently be broken and atm not possible)
If user adds a custom browser string, and EMTEST_BROWSER_AUTO_CONFIG is not enabled, then the browser might run in a tab on an existing browser, in which case this detection will not work.
test/common.py
Outdated
| # --fresh: do not restore old tabs (e.g. if user had old navigated windows open) | ||
| # --background: Open the new Safari window behind the current Terminal window, to make following the test run more pleasing (this is for convenience only) | ||
| # -a <exe_name>: The path to the executable to open, in this case Safari | ||
| browser_args = ['open', '--new', '--fresh', '--background', '-a'] + browser_args |
There was a problem hiding this comment.
Shouldn't these args go in SafariConfig?
There was a problem hiding this comment.
Or maybe these should be used also for Chrome and FF on macOS? i.e. is open the way we should be launching all browsers on macOS?
There was a problem hiding this comment.
It'll need a new construct in the configs, there is no current way to model a prefix of args in the existing set of config fields. I can do that in a moment.
Using open is not needed for Firefox at least. Not sure about Chrome, but I presume not.
test/common.py
Outdated
| else: | ||
| exit_with_error(f'EMTEST_BROWSER_AUTO_CONFIG only currently works with firefox or chrome. EMTEST_BROWSER was "{EMTEST_BROWSER}"') | ||
| if not config: | ||
| exit_with_error(f'EMTEST_BROWSER_AUTO_CONFIG only currently works with firefox, chrome and safari. EMTEST_BROWSER was "{EMTEST_BROWSER}"') |
There was a problem hiding this comment.
This error seems like its unreachable since its inside the if EMTEST_BROWSER_AUTO_CONFIG and config block?
|
This would be great to land, so I can get on to running tests on the Safari browser next. |
There was a problem hiding this comment.
Added @brendandahl who has also been working on this stuff recently.
lgtm, even though all this extra complexity is making me sad.
|
You know what they say, better correct and sad, rather than end users mad. |
test/common.py
Outdated
| return EMTEST_BROWSER and 'safari' in EMTEST_BROWSER.lower() | ||
|
|
||
|
|
||
| def get_safari_version(): |
There was a problem hiding this comment.
How about get_version_from_macos_app_dir (or something like that?)
There was a problem hiding this comment.
Oh wait.. this function looks like it not actually used?
There was a problem hiding this comment.
Err, I was hoping you wouldn't notice :)
I used this function in the initial version of this PR. Initially I couldn't figure out why old Safari was failing some random tests. And it amounted to being that missing HTTP caching header Vary: * that was causing the failures.
So I could then remove the use of the version check.
But I have plans to use that Safari version check soon more once I get to test more coverage of the tests in different Safari versions. Instead of a @no_safari('reason') annotation, I would like to annotate tests with @no_safari_older_than(XXYYZZ, 'reason') so that the test skips are not that blunt and oblivious to the version. That way they can naturally evolve out when minimum supported Safari version evolves.
If you'd like, I can also reintroduce that function in a later PR that actually adds uses of such pattern. (I am planning to introduce a symmetric get_firefox_version() function as well, and instead of @no_firefox, to have @no_firefox_older_than checks)
There was a problem hiding this comment.
Can maybe run something like browser-binary --version in all cases? Then the same code work everywhere?
Lets remove this PR since it also introduces that new plist import thing.
There was a problem hiding this comment.
Its a shame the new vulture check isn't able to be sure about this code being dead so it doesn't fail in the CI step :(
Add support for running the
browserharness withEMTEST_BROWSER=/Applications/Safari.App/Contents/MacOS/Safarion a macOS system.This fixes both the singlethreaded and multithreaded harness runs.