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

Rewrite /wpt/cookies/http-state/attribute.html tests #26578

Merged
merged 1 commit into from Nov 25, 2020

Conversation

@chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Nov 19, 2020

This is the first of a handful of CLs rewriting the legacy
tests in the ported wpt http-state tests. The plan is to continue
to refine and refactor and eventually rewrite (or delete) them all.

Bug: 1145300
Change-Id: I8a50939f4e2c95c1293ba5423577693ab2a10d9a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2550362
Commit-Queue: Mike Taylor <miketaylr@chromium.org>
Auto-Submit: Mike Taylor <miketaylr@chromium.org>
Reviewed-by: Lily Chen <chlily@chromium.org>
Cr-Commit-Position: refs/heads/master@{#830803}

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

The review process for this patch is being conducted in the Chromium project.

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-2550362 branch from cc389f6 to bc27f4e Nov 23, 2020
@wpt-pr-bot wpt-pr-bot added the infra label Nov 23, 2020
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-2550362 branch 2 times, most recently from b6ce61e to a27c669 Nov 23, 2020
@miketaylr
Copy link
Contributor

@miketaylr miketaylr commented Nov 23, 2020

Relates to #26190

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-2550362 branch from a27c669 to 5c76513 Nov 24, 2020
@miketaylr
Copy link
Contributor

@miketaylr miketaylr commented Nov 24, 2020

@jgraham could you please re-trigger/re-run the wpt-firefox-nightly-stability check on TC?

Oh, false alarm. It's re-running now (I guess cause CQ is trying the final patchset).

Client ID github/67283|miketaylr does not have sufficient scopes

Screen Shot 2020-11-23 at 6 06 07 PM

@miketaylr
Copy link
Contributor

@miketaylr miketaylr commented Nov 24, 2020

OK, I guess it's still flaky. Will investigate.

@miketaylr
Copy link
Contributor

@miketaylr miketaylr commented Nov 24, 2020

OK, I can reproduce locally with the following:

git fetch origin && git checkout -b chromium-export-cl-2550362 origin/chromium-export-cl-2550362
./wpt install --channel nightly firefox browser
./wpt install --channel nightly firefox webdriver

python3 ./wpt run --channel=nightly --verify --verify-no-chaos-mode --verify-repeat-loop=0 --verify-repeat-restart=10 --log-mach-level=info --log-mach=- -y --no-pause --no-restart-on-unexpected --no-headless --verify-log-full --binary="/Users/miketaylr/dev/wpt/_venv2/browsers/nightly/Firefox Nightly.app/Contents/MacOS/firefox" firefox cookies/attributes

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-2550362 branch from 5c76513 to 91f46f2 Nov 24, 2020
@miketaylr
Copy link
Contributor

@miketaylr miketaylr commented Nov 24, 2020

To the best of my knowledge, the issue is related to trying to run promise tests that set and get cookies while opening a new window (via window.open()) which runs its own cookie tests that set and get cookies.

In Firefox, this works most of the time, but as the stability run shows, it can be flaky. Locally, I could get it to reproduce once every 3 or 4 refreshes. Either the cookies don't get set (unlikely, probably?), or there's a race and the get happens before the set, so document.cookie returns an empty string.

Pulling the child window tests into its own file resolves the issue for me (and is probably a better idea, in retrospect).

I'm not sure if this is related to what Ehsan is describing here, https://bugzilla.mozilla.org/show_bug.cgi?id=1331680#c2, or just bad test design on my side of things. But it's something related to the architecture of Firefox (or it's tickling a bug), as Chromium seems fine with it either way.

0:42.65 INFO ### /cookies/attributes/secure.https.html ###
 0:42.65 INFO |                Subtest                 | Results | Messages |
 0:42.65 INFO |----------------------------------------|---------|----------|
 0:42.65 INFO |                                        | OK      |          |
 0:42.65 INFO | `Set cookie for Secure attribute`      | PASS    |          |
 0:42.65 INFO | `Set cookie for seCURe attribute`      | PASS    |          |
 0:42.65 INFO | `Set cookie for for Secure= attribute` | PASS    |          |
 0:42.65 INFO | `Set cookie for Secure=aaaa`           | PASS    |          |
 0:42.65 INFO | `Set cookie for Secure space equals`   | PASS    |          |
 0:42.65 INFO | `Set cookie for Secure equals space`   | PASS    |          |
 0:42.65 INFO | `Set cookie for spaced Secure`         | PASS    |          |
 0:42.65 INFO | `Set cookie for space Secure with ;`   | PASS    |          |
 0:42.65 INFO 
 0:42.65 INFO ::: Running tests in a loop with restarts 10 times : PASS
 0:42.65 INFO :::
 0:42.65 INFO ::: Test verification PASS
 0:42.65 INFO :::
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-2550362 branch from 91f46f2 to 640e945 Nov 24, 2020
@miketaylr
Copy link
Contributor

@miketaylr miketaylr commented Nov 24, 2020

Yep, that fixed it.

This is the first of a handful of CLs rewriting the legacy
tests in the ported wpt http-state tests. The plan is to continue
to refine and refactor and eventually rewrite (or delete) them all.

Bug: 1145300
Change-Id: I8a50939f4e2c95c1293ba5423577693ab2a10d9a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2550362
Commit-Queue: Mike Taylor <miketaylr@chromium.org>
Auto-Submit: Mike Taylor <miketaylr@chromium.org>
Reviewed-by: Lily Chen <chlily@chromium.org>
Cr-Commit-Position: refs/heads/master@{#830803}
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-2550362 branch from 640e945 to a28f4d0 Nov 25, 2020
@chromium-wpt-export-bot chromium-wpt-export-bot merged commit 20b20fa into master Nov 25, 2020
22 checks passed
22 checks passed
Azure Pipelines Build #20201125.5 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
staging.wpt.fyi - chrome[experimental] Chrome results
Details
staging.wpt.fyi - firefox[experimental] Firefox results
Details
staging.wpt.fyi - safari[experimental] Safari results
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
@chromium-wpt-export-bot chromium-wpt-export-bot deleted the chromium-export-cl-2550362 branch Nov 25, 2020
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

3 participants