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

Change async_test to promise_test in clear-window-name.https.html #25572

Merged
merged 2 commits into from Sep 23, 2020

Conversation

@stephenmcgruer
Copy link
Contributor

@stephenmcgruer stephenmcgruer commented Sep 16, 2020

These all appear to be straight-forward promise tests, not async tests.

See #21435

@stephenmcgruer stephenmcgruer marked this pull request as ready for review Sep 16, 2020
@wpt-pr-bot wpt-pr-bot added the html label Sep 16, 2020
@stephenmcgruer
Copy link
Contributor Author

@stephenmcgruer stephenmcgruer commented Sep 16, 2020

There's a slight performance hit here by not running them concurrently (now takes ~2.5s on Chrome and Firefox on CI, versus 1 and 1.8s before). If we care about that, we can make these properly async_tests by not using await (they don't actually need it, but this was the easy fix).

Weirdly, there's a diff in Safari that I haven't figured out yet - https://wpt.fyi/results/html/browsers/windows/clear-window-name.https.html?diff&filter=ADC&run_id=688120008&run_id=669410010 . This test may be flaky on Safari (it was just added, and doesn't have enough wpt.fyi runs to figure that out), or I may have inadvertently triggered some sort of behavior change. Perhaps you can try running it locally @foolip to see if you can reproduce with/without?

@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-25572 Sep 16, 2020 Inactive
@annevk
Copy link
Member

@annevk annevk commented Sep 18, 2020

Sorry for missing this in review. This looks reasonable to me. @artines1 might have thoughts as the original author.

@annevk
annevk approved these changes Sep 18, 2020
@stephenmcgruer
Copy link
Contributor Author

@stephenmcgruer stephenmcgruer commented Sep 18, 2020

Sorry for missing this in review. This looks reasonable to me. @artines1 might have thoughts as the original author.

No worries, I assigned to foolip anyway ;)

I'll gives artines1 24 hours to see if they have any comments, then I'll merge this.

@artines1
Copy link
Contributor

@artines1 artines1 commented Sep 18, 2020

The patch looks good to me. I don't have any comments. Thanks.

@foolip
Copy link
Member

@foolip foolip commented Sep 18, 2020

@stephenmcgruer I've tried running ./wpt run --no-pause --channel=preview safari html/browsers/windows/clear-window-name.https.html at commit e0020b8 (without this change) and subtest "Window.name is set by the window" times out every time. It takes 60 seconds, so I only tried 3 times.

@stephenmcgruer
Copy link
Contributor Author

@stephenmcgruer stephenmcgruer commented Sep 18, 2020

@foolip then it seems like I have introduced a behavior change (probably by making it non-concurrent), but unclear to me what. If you're able to do any debugging that would be great, there may be a Safari bug hiding here? (Something about simultaneous windows being opened...?)

@foolip
Copy link
Member

@foolip foolip commented Sep 18, 2020

@stephenmcgruer when running the tests with your change, it all happens very fast. Windows open and close very fast and then the test is done. Before, one window was just left sitting open.

I've not been able to get to the bottom of it, but it looks like what matters is what happens after the hanging test. By removing all the prior tests (leaving just the last two) and leaving just the call to anchorClick in the second test, the hang still repros. The problem isn't in pollResultAndCheck, but something about clicking to open another window after "Window.name is set by the window" breaks the test. It doesn't seem to matter what URL is opened in the second test.

I'd need to spend more time minifying this, more time than I have today.

These all appear to be straight-forward promise tests, not async tests.

See #21435
@foolip
foolip approved these changes Sep 22, 2020
Copy link
Member

@foolip foolip left a comment

Almost LGTM. Approving so you can merge after fixing nits.

@stephenmcgruer
Copy link
Contributor Author

@stephenmcgruer stephenmcgruer commented Sep 23, 2020

@foolip - we still have the surprising improvement in Safari that I don't understand here. Are you still happy to merge this?

@annevk
Copy link
Member

@annevk annevk commented Sep 23, 2020

Safari reuses the top-level window when you use <a>.click(). That explains the difference. That doesn't really seem non-conforming so this seems like a better way to test this.

@stephenmcgruer
Copy link
Contributor Author

@stephenmcgruer stephenmcgruer commented Sep 23, 2020

Safari reuses the top-level window when you use .click(). That explains the difference. That doesn't really seem non-conforming so this seems like a better way to test this.

Ack, thanks!

@stephenmcgruer stephenmcgruer merged commit d8e451a into master Sep 23, 2020
21 checks passed
21 checks passed
detect-deployment
Details
Azure Pipelines Build #20200921.33 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
download-firefox-nightly Community-TC (pull_request)
Details
lint Community-TC (pull_request)
Details
sink-task Community-TC (pull_request)
Details
update-built Community-TC (pull_request)
Details
wpt-chrome-dev-results Community-TC (pull_request)
Details
wpt-chrome-dev-results-without-changes Community-TC (pull_request)
Details
wpt-chrome-dev-stability Community-TC (pull_request)
Details
wpt-decision-task Community-TC (pull_request)
Details
wpt-firefox-nightly-results Community-TC (pull_request)
Details
wpt-firefox-nightly-results-without-changes Community-TC (pull_request)
Details
wpt-firefox-nightly-stability Community-TC (pull_request)
Details
wpt.fyi - chrome[experimental] Chrome results
Details
wpt.fyi - firefox[experimental] Firefox results
Details
wpt.fyi - safari[experimental] Safari results
Details
@stephenmcgruer stephenmcgruer deleted the smcgruer/async_async branch Sep 23, 2020
@foolip
Copy link
Member

@foolip foolip commented Sep 24, 2020

Ah, thanks for explaining what's going on @annevk!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.