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

[ResourceTiming] Add test for new TAO check #20320

Merged
merged 2 commits into from Nov 19, 2019
Merged

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

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

This CL modifies the tests as follows:

  • '' is now the default TAO header. This means most sandwich tests use ''.
  • Another resource is added to crossorigin-sandwich-TAO to test the case in
    which the header is the page origin, and this should now fail (this was the
    test before this CL).
  • The resources in crossorigin-sandwich-TAO are changed to images because it's
    not super clear how iframes will be handled.

Bug: 1022816
Change-Id: I2110653a5240cff825d3130cfe6dc64e8d2f8e23
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1924635
Commit-Queue: Nicolás Peña Moreno <npm@chromium.org>
Reviewed-by: Yoav Weiss <yoavweiss@chromium.org>
Cr-Commit-Position: refs/heads/master@{#716652}

Copy link
Collaborator

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

Choose a reason for hiding this comment

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

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

This CL modifies the tests as follows:
* '*' is now the default TAO header. This means most sandwich tests use '*'.
* Another resource is added to crossorigin-sandwich-TAO to test the case in
which the header is the page origin, and this should now fail (this was the
test before this CL).
* The resources in crossorigin-sandwich-TAO are changed to images because it's
not super clear how iframes will be handled.

Bug: 1022816
Change-Id: I2110653a5240cff825d3130cfe6dc64e8d2f8e23
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1924635
Commit-Queue: Nicolás Peña Moreno <npm@chromium.org>
Reviewed-by: Yoav Weiss <yoavweiss@chromium.org>
Cr-Commit-Position: refs/heads/master@{#716652}
@npm1
Copy link
Contributor

npm1 commented Nov 19, 2019

Ok, I did not see that this is complaining and the Chromium patch landed, woops. Some things:

  • Looks like the commit message gets copy-pasted, which in this case results in some deformation because for example * is parsed differently.
  • The failure seems to be caused by duplicate test message "fetchStart > 0 in cross-origin redirect.". Was there a way to catch this problem without looking at the GitHub PR? I forgot this was a rule...

@foolip for help! If my reading is right and the problem is just the test messages, I can fix that but I don't want to unsync the chromium and GitHub version further.

@Hexcles
Copy link
Member

Hexcles commented Nov 19, 2019

Chatted with @npm1 . The test_* helper functions generate subtests (they call test at the end), not just assertions, so the third argument is actually the test name not the message.

@npm1
Copy link
Contributor

npm1 commented Nov 19, 2019

Ah still failing... @Hexcles the Firefox failure seems to be caused by Firefox computing separate timestamps which means sometimes the values will be close but not equal and other times the values will be equal (so the test is considered flaky). Can we force-merge in this case?

@Hexcles
Copy link
Member

Hexcles commented Nov 19, 2019

Firefox computing separate timestamps which means sometimes the values will be close but not equal and other times the values will be equal

This sounds to me like an implementation problem. I'm not a domain expert so I'll trust your judgement here (file a bug to Firefox if needed.)

@Hexcles Hexcles merged commit cd51095 into master Nov 19, 2019
@Hexcles Hexcles deleted the chromium-export-cl-1924635 branch November 19, 2019 18:48
@foolip
Copy link
Member

foolip commented Nov 27, 2019

Thanks @Hexcles for helping out with this!

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.

None yet

5 participants