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

[resource timing] Improve document.domain tests (reland) #28965

Merged
merged 2 commits into from
May 12, 2021

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented May 12, 2021

This CL is a fixed up reland of [1], which was reverted due to a rename
in entry-invariants.js that it was relying on.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/2878852

Bug: 1171767, 1208054
Change-Id: I436bf6973e4399d142ce45e77c07f26d0e7e1cd1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2888667
Reviewed-by: Tom McKee <[email protected]>
Commit-Queue: Yoav Weiss <[email protected]>
Cr-Commit-Position: refs/heads/master@{#881945}

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 is a fixed up reland of [1], which was reverted due to a rename
in entry-invariants.js that it was relying on.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/2878852

Bug: 1171767, 1208054
Change-Id: I436bf6973e4399d142ce45e77c07f26d0e7e1cd1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2888667
Reviewed-by: Tom McKee <[email protected]>
Commit-Queue: Yoav Weiss <[email protected]>
Cr-Commit-Position: refs/heads/master@{#881945}
@yoavweiss
Copy link
Contributor

yoavweiss commented May 12, 2021

Looking at the Chrome and Firefox logs, I don't see any issue with the test this actually adds... Halp?

@KyleJu
Copy link
Contributor

KyleJu commented May 12, 2021

Looking at the Chrome and Firefox logs, I don't see any issue with the test this actually adds... Halp?

Looking at the logs, it looks like that unrelated tests are triggered and run, which led to timeout. I think it is safe to force merge.

@KyleJu
Copy link
Contributor

KyleJu commented May 12, 2021

@jpchase timeout on Firefox and Chrome and seems to be unrelated. Could you admin merge?

@jpchase
Copy link

jpchase commented May 12, 2021

@jpchase timeout on Firefox and Chrome and seems to be unrelated. Could you admin merge?

I would agree, except the reverted version of the PR (#28901) also had timeouts that appear to be similar:
https://github.com/web-platform-tests/wpt/pull/28901/checks?check_run_id=2558118969

Meanwhile, other PRs have recently ran successfully (e.g. #28963).

@KyleJu would you rebase and rerun the checks on this PR? I'd like to see if that can rule out the timeout being somehow related to these changes (even though we don't have reason to suspect so).

@KyleJu
Copy link
Contributor

KyleJu commented May 12, 2021

@jpchase timeout on Firefox and Chrome and seems to be unrelated. Could you admin merge?

I would agree, except the reverted version of the PR (#28901) also had timeouts that appear to be similar:
https://github.com/web-platform-tests/wpt/pull/28901/checks?check_run_id=2558118969

Meanwhile, other PRs have recently ran successfully (e.g. #28963).

Note that Chrome/Firefox checks are running against PR changes. so #28963 is probably not indicative of the status of checks in this PR because they are running different sets of tests. I am suspecting common/get-host-info.sub.js is the reason why so many unrelated tests are triggered and run

@KyleJu would you rebase and rerun the checks on this PR? I'd like to see if that can rule out the timeout being somehow related to these changes (even though we don't have reason to suspect so).

Yea rebase and rerun sound like a good idea.

@foolip
Copy link
Member

foolip commented May 12, 2021

Perhaps there are multiple things going on here, but I see at least some of the stability jobs have timed out, which is #7660. When this happens, I just link to that issue and admin merge.

@past FYI, this is a pretty common source of issues, and pretty high up on my list of things we should fix in WPT CI.

@KyleJu
Copy link
Contributor

KyleJu commented May 12, 2021

@jpchase Still timing out. I think we can admin merge this PR

@jpchase
Copy link

jpchase commented May 12, 2021

@jpchase Still timing out. I think we can admin merge this PR

Yes, agreed to admin merge.

@jpchase jpchase merged commit a45d504 into master May 12, 2021
@jpchase jpchase deleted the chromium-export-cl-2888667 branch May 12, 2021 21:49
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.

6 participants