Skip to content

Validate pull requests in TaskCluster #12657

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.

Sign up for GitHub

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

Merged
jgraham merged 20 commits into from
Sep 12, 2018
Merged

Validate pull requests in TaskCluster #12657

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
541acf3
[infra] Validate pull requests in TaskCluster
jugglinmike Aug 7, 2018
c3f109b
[infra] Temporarily ignore stability status in CI
jugglinmike Aug 23, 2018
a1f01d0
Demonstrate task with zero affected tests
jugglinmike Aug 24, 2018
ee8d57d
DO NOT MERGE Introduce instability
jugglinmike Aug 23, 2018
7808fb1
Re-implement script in Python
jugglinmike Aug 29, 2018
0d6c2a2
Reduce repetition
jugglinmike Aug 29, 2018
c1792ee
Trigger TaskCluster
jugglinmike Aug 29, 2018
f422774
Trigger CI
jugglinmike Aug 29, 2018
150058e
Correct commit range
jugglinmike Aug 29, 2018
0b623f3
Trigger CI
jugglinmike Aug 29, 2018
30b4ae4
Incorporate review feedback
jugglinmike Aug 31, 2018
3371a4c
Test at GitHub-provided merge commit
jugglinmike Aug 31, 2018
cbc779c
Simplify reference
jugglinmike Aug 31, 2018
887512c
Remove superflous command
jugglinmike Aug 31, 2018
d9a12c3
Revert "Remove superflous command"
jugglinmike Aug 31, 2018
80714c9
Remove superfluous argument
jugglinmike Sep 6, 2018
c4ea324
Accommodate jobs with zero affected tests
jugglinmike Sep 6, 2018
dba6e62
Merge branch 'master' into taskcluster-stability-2
jugglinmike Sep 6, 2018
5c46835
Merge branch 'master' into taskcluster-stability-2
jugglinmike Sep 11, 2018
e79a708
Revert "DO NOT MERGE Introduce instability"
jugglinmike Sep 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
191 changes: 138 additions & 53 deletions .taskcluster.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,42 +2,110 @@ version: 1
policy:
pullRequests: collaborators
tasks:
$if: tasks_for == "github-push"
then:
$if: event.ref == "refs/heads/master"
then:
$flattenDeep:
$flattenDeep:
- $if: tasks_for == "github-push"
then:
$if: event.ref == "refs/heads/master"
then:
$map: [{name: firefox, channel: nightly}, {name: chrome, channel: dev}]
each(browser):
$map:
- [testharness, 1, 15]
- [testharness, 2, 15]
- [testharness, 3, 15]
- [testharness, 4, 15]
- [testharness, 5, 15]
- [testharness, 6, 15]
- [testharness, 7, 15]
- [testharness, 8, 15]
- [testharness, 9, 15]
- [testharness, 10, 15]
- [testharness, 11, 15]
- [testharness, 12, 15]
- [testharness, 13, 15]
- [testharness, 14, 15]
- [testharness, 15, 15]
- [reftest, 1, 10]
- [reftest, 2, 10]
- [reftest, 3, 10]
- [reftest, 4, 10]
- [reftest, 5, 10]
- [reftest, 6, 10]
- [reftest, 7, 10]
- [reftest, 8, 10]
- [reftest, 9, 10]
- [reftest, 10, 10]
- [wdspec, 1, 1]
each(chunk):
taskId: {$eval: 'as_slugid(browser.name + browser.channel + chunk[0] + str(chunk[1]))'}
taskGroupId: {$eval: 'as_slugid("task group")'}
created: {$fromNow: ''}
deadline: {$fromNow: '24 hours'}
provisionerId: aws-provisioner-v1
# Contributors interested in configurating TaskCluster to run
# against their fork of WPT should change the `workerType` to
# "github-worker".
workerType: wpt-docker-worker
metadata:
name: wpt-${browser.name}-${browser.channel}-${chunk[0]}-${chunk[1]}
description: >-
A subset of WPT's "${chunk[0]}" tests (chunk number ${chunk[1]}
of ${chunk[2]}), run in the ${browser.channel} release of
${browser.name}.
owner: ${event.pusher.email}
source: ${event.repository.url}
payload:
image: jugglinmike/web-platform-tests:0.18

@jgraham jgraham Aug 24, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We really need to set up a shared dockerhub account :)

maxRunTime: 7200
artifacts:
public/results:
path: /home/test/artifacts
type: directory
command:
- /bin/bash
- --login
- -c
- set -ex;
~/start.sh
${event.repository.url}
${event.ref[len('refs/heads/'):]}
${event.after}
${browser.name}-${browser.channel};
cd ~/web-platform-tests;
./tools/ci/ci_taskcluster.sh
--test-type=${chunk[0]}
--this-chunk=${chunk[1]}
--total-chunks=${chunk[2]}
${browser.name};
- $if: tasks_for == "github-pull-request"
then:
$map: [{name: firefox, channel: nightly}, {name: chrome, channel: dev}]
each(browser):
$map:
- [testharness, 1, 15]
- [testharness, 2, 15]
- [testharness, 3, 15]
- [testharness, 4, 15]
- [testharness, 5, 15]
- [testharness, 6, 15]
- [testharness, 7, 15]
- [testharness, 8, 15]
- [testharness, 9, 15]
- [testharness, 10, 15]
- [testharness, 11, 15]
- [testharness, 12, 15]
- [testharness, 13, 15]
- [testharness, 14, 15]
- [testharness, 15, 15]
- [reftest, 1, 10]
- [reftest, 2, 10]
- [reftest, 3, 10]
- [reftest, 4, 10]
- [reftest, 5, 10]
- [reftest, 6, 10]
- [reftest, 7, 10]
- [reftest, 8, 10]
- [reftest, 9, 10]
- [reftest, 10, 10]
- [wdspec, 1, 1]
each(chunk):
taskId: {$eval: 'as_slugid(browser.name + browser.channel + chunk[0] + str(chunk[1]))'}
- name: wpt-${browser.name}-${browser.channel}-stability
description: >-
Verify that all tests affected by a pull request are stable
when executed in ${browser.name}. As of 2018-08-23, this task

@jgraham jgraham Aug 24, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove the extra text from the description; I don't think it's that helpful (we're more likely to forget to remove it later).

@jugglinmike jugglinmike Aug 29, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'd like there to be some contributor-facing indication of this behavior.
I appreciate the concern about bitrot, though, so I've moved it to a statement
logged to standard out immediately following the command invocation. Does that
seem safe enough to you?

is being vetted for correctness, so its outcome will not
influence the status of the pull request.
command:
status=PASS;
echo $affected |
base64 -d |
xargs -0 ./tools/ci/ci_taskcluster.sh --verify ${browser.name} || status=FAIL;
echo $status > ../artifacts/stability-status.txt
creates_artifacts: true
- name: wpt-${browser.name}-${browser.channel}-results
description: >-
Collect results for all tests affected by a pull request in
${browser.name}.
command:
echo $affected |
base64 -d |
xargs -0 ./tools/ci/ci_taskcluster.sh ${browser.name}
creates_artifacts: true
each(operation):
taskId: {$eval: 'as_slugid(operation.name)'}
taskGroupId: {$eval: 'as_slugid("task group")'}
created: {$fromNow: ''}
deadline: {$fromNow: '24 hours'}
Expand All @@ -47,31 +115,48 @@ tasks:
# "github-worker".
workerType: wpt-docker-worker
metadata:
name: wpt-${browser.name}-${browser.channel}-${chunk[0]}-${chunk[1]}
description: >-
A subset of WPT's "${chunk[0]}" tests (chunk number ${chunk[1]}
of ${chunk[2]}), run in the ${browser.channel} release of
${browser.name}.
owner: ${event.pusher.email}
name: ${operation.name}
description: ${operation.description}
owner: ${event.pull_request.user.login}@users.noreply.github.com
Comment thread
jgraham marked this conversation as resolved.
source: ${event.repository.url}
payload:
image: gsnedders/web-platform-tests:0.13
image: jugglinmike/web-platform-tests:0.18
maxRunTime: 7200
artifacts:
public/results:
path: /home/test/artifacts
type: directory
$if: operation.creates_artifacts
then:
public/results:
path: /home/test/artifacts
type: directory
# Bash removes null bytes from string values when set as
# environment variables. This invalidates the output of `wpt
# affected-tests` because it uses the null byte as a separator
# between test names. The list of effected tests is

@jgraham jgraham Aug 24, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That should be affected. But this complexity seems like a clue that a better solution is required.

# Base64-encoded in order to preserve the null bytes.
command:
- /bin/bash
- --login
- -c
- "~/start.sh ${event.repository.url} ${event.ref[len('refs/heads/'):]} ${event.after} ${browser.name}-${browser.channel} &&
cd ~/web-platform-tests &&
./tools/ci/ci_taskcluster.sh ${browser.name} ${chunk[0]} ${chunk[1]} ${chunk[2]}"
else: []
# > NOTE: A well-designed template should produce `tasks: []` for any
# > unrecognized `task_for` values; this allows later expansion of this
# > service to handle more events.
#
# https://docs.taskcluster.net/docs/reference/integrations/taskcluster-github/docs/taskcluster-yml-v1
else: []
- set -ex;
~/start.sh
${event.repository.clone_url}
${event.pull_request.base.ref}
${event.pull_request.base.sha}
${browser.name}-${browser.channel};
cd ~/web-platform-tests;
./wpt manifest-download || true;
git fetch
${event.repository.clone_url}
refs/pull/${event.number}/head;
git checkout FETCH_HEAD;
affected=$(./wpt tests-affected
--null
${event.pull_request.base.sha}...
| base64 --wrap 0);
if [ -z $(echo $affected | base64 -d) ]; then
echo Zero tests affected.;
exit 0;
fi;
echo Affected tests:;
echo $affected | base64 -d | sed 's/\x0/\n/g';
${operation.command}
4 changes: 4 additions & 0 deletions dom/events/CustomEvent.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@
<script src="/resources/testharnessreport.js"></script>
<div id="log"></div>
<script>
test(function() {
assert_true(Math.random() > 0.5);
}, 'intentionally unstable test');

test(function() {
var type = "foo";

Expand Down
44 changes: 39 additions & 5 deletions tools/ci/ci_taskcluster.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,43 @@

./wpt manifest-download

if [ $1 == "firefox" ]; then
./wpt run firefox --log-tbpl=../artifacts/log_tbpl.log --log-tbpl-level=info --log-wptreport=../artifacts/wpt_report.json --log-mach=- --this-chunk=$3 --total-chunks=$4 --test-type=$2 -y --install-browser --no-pause --no-restart-on-unexpected --reftest-internal --install-fonts --no-fail-on-unexpected
elif [ $1 == "chrome" ]; then
./wpt run chrome --log-tbpl=../artifacts/log_tbpl.log --log-tbpl-level=info --log-wptreport=../artifacts/wpt_report.json --log-mach=- --this-chunk=$3 --total-chunks=$4 --test-type=$2 -y --no-pause --no-restart-on-unexpected --install-fonts --no-fail-on-unexpected
# The first script argument that is not prefixed with a dash (`-`) is assumed
# to be the name of the browser under test. This restricts the syntax available
# to consumers: value-accepting options must be specified using the equals sign
# (`=`).
for argument in $@; do

@jgraham jgraham Aug 24, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I feel like this exceeded my complexity threshold for a bash script. If we want a single entry point for all the things, let's make it a python script instead.

if [ ${argument:0:1} = '-' ]; then
continue
fi

browser_name=$argument

break
done

browser_specific_args=''

if [ $browser_name = 'firefox' ]; then
browser_specific_args='--install-browser --reftest-internal'
fi
gzip ../artifacts/wpt_report.json

./wpt run \
--log-tbpl=../artifacts/log_tbpl.log \
--log-tbpl-level=info \
--log-wptreport=../artifacts/wpt_report.json \
--log-mach=- \
-y \
--no-pause \
--no-restart-on-unexpected \
--install-fonts \
--no-fail-on-unexpected \
$browser_specific_args \
$@

run_status=$?

if [ -f ../artifacts/wpt_report.json ]; then
gzip ../artifacts/wpt_report.json
fi

exit $run_status
6 changes: 6 additions & 0 deletions tools/docker/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,4 +69,10 @@ WORKDIR /home/test/

COPY .bashrc /home/test/.bashrc

# Ubuntu inserts a logout script which exits with a non-zero status. This makes
# it impossible to use the `exit` built-in from a login shell where the `-e`
# option has been set. Since the script is intended to clear the console for
# interactive sessions, it may safely be removed.
RUN rm /home/test/.bash_logout

COPY start.sh /home/test/start.sh