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

Execute affected tests both with and without a PR's changes #708

Closed
lukebjerring opened this issue Oct 30, 2018 · 6 comments
Closed

Execute affected tests both with and without a PR's changes #708

lukebjerring opened this issue Oct 30, 2018 · 6 comments
Assignees

Comments

@lukebjerring
Copy link
Contributor

This will allow a more apples-to-apples diff than comparing to a recent full run of the same product (see #707).

@lukebjerring
Copy link
Contributor Author

This (executing both with and without) is a better solution to a problem that was mentioned in #745 - namely, as an interim we are comparing a PR's affected tests againts "the latest master run".
This is done with a link that points to chrome[master], for example - a link that will change over time (as new master runs arrive).

cc @foolip

@foolip
Copy link
Member

foolip commented Nov 16, 2018

@lukebjerring, can you send a PR for the Taskcluster changes? Here's what I'd planned to do for Azure Pipelines:

  • In a "decision task" make note of what HEAD^1 and HEAD^2 of the synthetic merge commit (refs/pull/1234/merge) are, call them SAVED_MASTER and SAVED_PR_HEAD. In particular the first parent is a commit on master, and master may change.
  • In the "with changes" task, check out SAVED_MASTER and merge in SAVED_PR_HEAD. Affected tests are ./wpt tests-affected $SAVED_MASTER.
  • In the "without changes" task check out SAVED_MASTER and merge in SAVED_PR_HEAD, call that commit TMP_MERGE. Then check out SAVED_MASTER and test that. Affected tests are ./wpt tests-affected $TMP_MERGE.

This is tragically convoluted, but I don't see how else to ensure differences between the two states of the trees were tested are only the changes from the PRs, and not also some random contribution of changes to master that happened as the tasks were queuing.

@jgraham may have thouhts.

@foolip
Copy link
Member

foolip commented Nov 16, 2018

I sent web-platform-tests/wpt#14093 for a thing I found confusing about the affected tests logic.

I also just conducted an experiment on foolip/wpt to fetch git fetch https://github.com/foolip/wpt.git refs/pull/12/merge && git rev-parse FETCH_HEAD, push commits to master, and fetch it again. FETCH_HEAD did change just seconds after. So there's nothing that GitHub does to provide stability here, and honestly I can't imagine what that could be.

@Hexcles
Copy link
Member

Hexcles commented Dec 6, 2018

I also just conducted an experiment on foolip/wpt to fetch git fetch https://github.com/foolip/wpt.git refs/pull/12/merge && git rev-parse FETCH_HEAD, push commits to master, and fetch it again. FETCH_HEAD did change just seconds after. So there's nothing that GitHub does to provide stability here, and honestly I can't imagine what that could be.

This is a well known but undocumented feature: https://discourse.drone.io/t/github-claims-that-merge-refs-are-undocumented-feature/1100

It's so widely used (especially by CI services including Travis) that I don't think it will go away. That said, we shouldn't rely too heavily on the timeliness (usually, the merge ref is updated shortly after the PR is updated). Another caveat is that when there is a merge conflict, the merge ref is invalid.

@Hexcles
Copy link
Member

Hexcles commented Dec 6, 2018

I just sent a PR to modify the Taskcluster configuration of wpt to run affected tests without patch: web-platform-tests/wpt#14382

(Note that we don't really have a "decision task" yet. Each task executes independently, so various race conditions may happen, especially if a PR is being updated quickly, e.g. different merge refs in the Firefox & Chrome tasks. But I think it's good enough for now.)

In addition, we need to make some changes to wpt.fyi to accommodate this. #865 is the first step: collecting the "without patch" results. Then we can compare them against the "with patch" results.

@foolip
Copy link
Member

foolip commented Dec 6, 2018

It sounds like the undocumented feature is simply refs/pull/1234/merge, which is what Azure Pipelines depends on and presumably Taskcluster as well.

Hexcles added a commit to web-platform-tests/wpt that referenced this issue Dec 6, 2018
Hexcles added a commit to web-platform-tests/wpt that referenced this issue Dec 6, 2018
Hexcles added a commit that referenced this issue Dec 6, 2018
This change extends the set of valid task names to accept the
"*-results-without-changes" tasks that are being added in
web-platform-tests/wpt#14382 . The change also
adds a "pr_base" label to these special runs, and "pr_head" to the PR
runs with the changes to prepare for the next step: using these runs
to calculate PR regressions (not implemented in this change).

One more step towards #708 .
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Dec 14, 2018
…t the patch, a=testonly

Automatic update from web-platform-tests
Run affected tests on Taskcluster without changes

One step towards
web-platform-tests/wpt.fyi#708

--

wpt-commits: fc87d9b363525fdcc5b5053a7c9624614c3fd730
wpt-pr: 14382
mykmelez pushed a commit to mykmelez/gecko that referenced this issue Dec 15, 2018
…t the patch, a=testonly

Automatic update from web-platform-tests
Run affected tests on Taskcluster without changes

One step towards
web-platform-tests/wpt.fyi#708

--

wpt-commits: fc87d9b363525fdcc5b5053a7c9624614c3fd730
wpt-pr: 14382
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 3, 2019
…t the patch, a=testonly

Automatic update from web-platform-tests
Run affected tests on Taskcluster without changes

One step towards
web-platform-tests/wpt.fyi#708

--

wpt-commits: fc87d9b363525fdcc5b5053a7c9624614c3fd730
wpt-pr: 14382

UltraBlame original commit: 8fa9cbffeb35f59aad1e164576afbffe737cde8d
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 3, 2019
…t the patch, a=testonly

Automatic update from web-platform-tests
Run affected tests on Taskcluster without changes

One step towards
web-platform-tests/wpt.fyi#708

--

wpt-commits: fc87d9b363525fdcc5b5053a7c9624614c3fd730
wpt-pr: 14382

UltraBlame original commit: 8fa9cbffeb35f59aad1e164576afbffe737cde8d
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 3, 2019
…t the patch, a=testonly

Automatic update from web-platform-tests
Run affected tests on Taskcluster without changes

One step towards
web-platform-tests/wpt.fyi#708

--

wpt-commits: fc87d9b363525fdcc5b5053a7c9624614c3fd730
wpt-pr: 14382

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

No branches or pull requests

3 participants