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

Ability to run all tests for some PRs #13263

Closed
foolip opened this issue Sep 28, 2018 · 16 comments · Fixed by #18870
Closed

Ability to run all tests for some PRs #13263

foolip opened this issue Sep 28, 2018 · 16 comments · Fixed by #18870

Comments

@foolip
Copy link
Member

foolip commented Sep 28, 2018

In #11769 a handful of tests were broken accidentally. This was reported in #13204 and also some of the same tests were found to fail in an attempt to roll into Chromium.

 cookie-store/http_cookie_and_set_cookie_headers.tentative.https.html
 cookies/http-state/charset-tests.html
 eventsource/format-field-id-2.htm
 eventsource/format-field-id.htm
 fetch/api/headers/header-values.html

Relatedly, many changes in resources/ by @lukebjerring, @jugglinmike and me have been put through Chromium's CQ first to see if they would break existing tests.

To help prevent accidental regressions and make it easier to look for anticipated regressions, the ability to run all tests in Taskcluster for some PRs would be very good.

Strawman: including the magic string "Taskcluster-Full-Run" somewhere would cause Taskcluster to treat all tests as affected.

Lots of similar solutions might be possible.

@jgraham

@foolip foolip added the infra label Sep 28, 2018
@jgraham
Copy link
Contributor

jgraham commented Sep 28, 2018

This is probably implementable right away, but reasonably we should make a proper decision task if we're going to introduce this kind of complexity.

@foolip
Copy link
Member Author

foolip commented Sep 28, 2018

Ah. And that requires switching away from .taskcluster.yml and using code to create tasks, I guess?

@gsnedders
Copy link
Member

Note that if we want to alert people to regressing tests (#7475) this may end up noisy for tools changes as long as there are flaky tests (and noisy to a far bigger degree than for normal PRs, given it'll expose flake in any test).

That said, I think this is probably worthwhile doing, given it's mostly the same work as needed to migrate the rest of our testing infrastructure to TC.

@foolip
Copy link
Member Author

foolip commented Dec 13, 2018

This would have come very much in handy to avoid #14495, if it were easy to do a full run we would do it.

@jgraham does this still depend on moving to a decision task? How much work would that be?

We should also give some consideration to how the feature would work on Azure Pipelines, if we want to also get full runs of Edge or Safari for PRs down the line.

@foolip
Copy link
Member Author

foolip commented Aug 29, 2019

Renaming to not be Taskcluster-specific, we want this for Azure Pipelines too.

@foolip foolip changed the title Ability to run all tests on Taskcluster for some PRs Ability to run all tests for some PRs Aug 29, 2019
@foolip
Copy link
Member Author

foolip commented Aug 29, 2019

On Azure Pipelines it would be fairly easy to do this based on something in the commit message.

@jgraham does Taskcluster have any declarative support for descison tasks yet, or is the situation unchanged?

@LukeZielinski
Copy link
Contributor

...from my duplicate bug - the only other suggestion is to potentially use a label instead of a tag in the PR or commit message.

@jgraham
Copy link
Contributor

jgraham commented Aug 29, 2019

The problem is not how to trigger it, the problem is how to ensure we get it chunked in a way that will actually finish and not waste resources.

@foolip
Copy link
Member Author

foolip commented Aug 29, 2019

If the trigger is easy, can't we trigger exactly the same tasks as we do for each push to master? Any other chunking would likely lead to some small number of differences in results due to side effects.

@gsnedders
Copy link
Member

There are some awkward questions here, right? Like:

  • Do we run the master/PR comparison runs?
  • Do we run the stability jobs?

If we're doing a full run there's a chance we're going to be touching loads of tests and all of those jobs will timeout.

@foolip
Copy link
Member Author

foolip commented Sep 3, 2019

My proposal is to trigger exactly the same setup as we do for full runs on master or epochs/* branches, and one would have to compare it against an existing run on wpt.fyi. In other words, one has to take care to base the PR on a commit for which there are results.

Stability jobs wouldn't be affected by this.

@foolip
Copy link
Member Author

foolip commented Sep 4, 2019

If anyone needs to do this manually now, 7378559 is how I triggered full Taskcluster runs to compare to others on master.

@foolip
Copy link
Member Author

foolip commented Sep 4, 2019

Proposal: let's have a branch name convention that triggers full runs, for example anything starting with fullrun/.

@gsnedders
Copy link
Member

That seems potentially annoying? Like, it'll end up with work happening on a branch "foo" and then needing to occasionally push to "fullrun/foo", unless you want every single commit to have a full run done.

@jgraham
Copy link
Contributor

jgraham commented Sep 4, 2019

I don't know if taskcluster.yml actually allows string prefix matches. But I think we could have a single branch that you force push to to get a full run pretty easily. As long as we're using the HEAD SHA from the event rather than the branch name, that should be pretty reliable I think.

I think this is a reasonable situation because:

  1. Full pushes should be rare. They are a big use of infrastructure resources and we only want people to trigger them when there's a clear need.
  2. It basically matches how (at least gecko) already works where you do a specific push to try to get a user-defined set of jobs run.
  3. It's clearly possible to implement without blocking on significant further work around job scheduling.

@foolip
Copy link
Member Author

foolip commented Sep 5, 2019

I've sent #18870 with one branch per configuration and documentation so that searching for "triggers/" will lead to something useful.

foolip added a commit that referenced this issue Sep 5, 2019
Handle empty list of commits in `get_extra_jobs`, since that's possible when force pushing.

Fixes #13263.
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Sep 12, 2019
…ering full runs, a=testonly

Automatic update from web-platform-tests
Add triggers/* branches to support triggering full runs (#18870)

Handle empty list of commits in `get_extra_jobs`, since that's possible when force pushing.

Fixes web-platform-tests/wpt#13263.
--

wpt-commits: d86745dade74feb7897b47eb9eca3c8b3c3d924d
wpt-pr: 18870
xeonchen pushed a commit to xeonchen/gecko that referenced this issue Sep 12, 2019
…ering full runs, a=testonly

Automatic update from web-platform-tests
Add triggers/* branches to support triggering full runs (#18870)

Handle empty list of commits in `get_extra_jobs`, since that's possible when force pushing.

Fixes web-platform-tests/wpt#13263.
--

wpt-commits: d86745dade74feb7897b47eb9eca3c8b3c3d924d
wpt-pr: 18870
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 4, 2019
…ering full runs, a=testonly

Automatic update from web-platform-tests
Add triggers/* branches to support triggering full runs (#18870)

Handle empty list of commits in `get_extra_jobs`, since that's possible when force pushing.

Fixes web-platform-tests/wpt#13263.
--

wpt-commits: d86745dade74feb7897b47eb9eca3c8b3c3d924d
wpt-pr: 18870

UltraBlame original commit: 04f26d751eeee27b6b7119be6712a786d2e90740
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 4, 2019
…ering full runs, a=testonly

Automatic update from web-platform-tests
Add triggers/* branches to support triggering full runs (#18870)

Handle empty list of commits in `get_extra_jobs`, since that's possible when force pushing.

Fixes web-platform-tests/wpt#13263.
--

wpt-commits: d86745dade74feb7897b47eb9eca3c8b3c3d924d
wpt-pr: 18870

UltraBlame original commit: 04f26d751eeee27b6b7119be6712a786d2e90740
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 4, 2019
…ering full runs, a=testonly

Automatic update from web-platform-tests
Add triggers/* branches to support triggering full runs (#18870)

Handle empty list of commits in `get_extra_jobs`, since that's possible when force pushing.

Fixes web-platform-tests/wpt#13263.
--

wpt-commits: d86745dade74feb7897b47eb9eca3c8b3c3d924d
wpt-pr: 18870

UltraBlame original commit: 04f26d751eeee27b6b7119be6712a786d2e90740
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants