-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Run master commits on Taskcluster #9226
Conversation
Build PASSEDStarted: 2018-03-16 17:35:33 View more information about this build on: |
5bb339f
to
696c6c3
Compare
696c6c3
to
ab20739
Compare
Interesting! @jugglinmike, very relevant to web-platform-tests/results-collection#535 (comment)
Wow. That'd achieve our wpt.fyi results are available within 1 hour for at least Firefox, and way sooner than I thought was possible. @lukebjerring @boaz @mariestaver @jugglinmike, this could change our roadmap drastically, so let's discuss at the earliest opportunity. |
@Hexcles, can you take a look at the bits around this, to see if it'll be a trivial integration with a wpt.fyi results POST endpoint? Seems like the token management and such would have to be very similar to what we've speculated about for PRs on Travis. |
.taskcluster.yml
Outdated
\ &&\n git config advice.detachedHead false &&\n git\ | ||
\ checkout {{event.head.sha}} &&\n ./tools/ci/ci_taskcluster.sh\ | ||
\ chrome reftest 1 6"] | ||
image: harjgam/web-platform-tests:0.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have a "web-platform-tests" organization on Docker Hub? Is that a thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is and we can; I just created an org with the namespace webplatformtests
; neither wpt
or web-platform-tests
worked, but I can't tell if they were taken or if there are some character/length restrictions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does the code live for creating the docker images?
@@ -0,0 +1,687 @@ | |||
allowPullRequests: collaborators |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This configuration file is huge and fairly repetitive. I assume it has to be checked in to the repo like .travis.yml, but could parts of it be generated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, there is a script :) Suggestion is then to add a comment at the top pointing to the script.
if [ $1 == "firefox" ]; then | ||
./wpt run firefox --log-tbpl=- --log-tbpl-level=debug --log-wptreport=../artifacts/wpt_report.json --this-chunk=$3 --total-chunks=$4 --test-type=$2 -y --install-browser --no-pause --no-restart-on-unexpected | ||
elif [ $1 == "chrome" ]; then | ||
./wpt run chrome --log-tbpl=- --log-tbpl-level=debug --log-wptreport=../artifacts/wpt_report.json --this-chunk=$3 --total-chunks=$4 --test-type=$2 -y --no-pause --no-restart-on-unexpected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which version of chrome will this be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on https://tools.taskcluster.net/groups/bLpNdT49RGCR7nEhELQE1A I'm guessing chrome dev, but I can't tell why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It gets installed using apt in tools/docker/start.sh
, pretty much like we do for travis. This probably isn't the optimal way to do this; the current setup is more of a proof of concept than anything, so I'm sure there are plenty of improvements we can make.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, well as long as it's not just accidentally chrome dev, that's just fine!
tools/docker/start.sh
Outdated
cd web-platform-tests | ||
git pull --depth=1 | ||
|
||
# Install Chome unstable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In #8838 I tried to banish "unstable" everywhere except package name, can you call it "Chrome dev"?
tools/docker/start.sh
Outdated
@@ -0,0 +1,27 @@ | |||
#!/bin/bash | |||
|
|||
sudo sh -c 'echo " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gsnedders wrote something to generate this output, can you use that here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
./wpt make-hosts-file
does this
tools/docker/Dockerfile
Outdated
|
||
RUN apt-get -qqy install \ | ||
firefox \ | ||
chromium-browser \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, the extra dependencies below are dependencies of Chrome (that you happen to know) but not of chromium-browser.
IMHO, since Chrome is already distributed in a deb package, we can perhaps skip this (i.e. only install firefox). See my next comment in start.sh
for dependency problems.
tools/docker/start.sh
Outdated
if ! sudo dpkg --install $deb_archive; then | ||
sudo apt-get -y install --fix-broken | ||
sudo dpkg --install $deb_archive | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is sufficient for solving Chrome's dependencies, so there's no need for installing chromium-browser in the Docker image.
Besides, you might want to try gdebi
, which installs a local deb package along with its dependencies (from apt). I think it's available by default on Ubuntu.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Hexcles This does the same, FWIW. (Also, BTW, you're not on IRC.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I know it's the same, but it'd save a couple lines :P
I'll step away from the detailed code review now as @Hexcles will do that, makes the most sense since it needs to be integrated together with the results upload endpoint, somehow. |
I have a question about browser version management. If each shard individually downloads https://dl.google.com/linux/direct/google-chrome-unstable_current_amd64.deb then once every few days it's possible they will not get the same version. At minimum it would be good if the browser version is recorded in the wptreport JSON so that we can detect this. Even better would be if the browser version is somehow managed together with the container. The very simplest would be to have the browsers installed in the container itself, which would mean updating the container daily, but that can be automated pretty easily. https://github.com/whatwg/html-build/blob/master/ci-deploy/outside-container.sh does this. How we then ensure that the same Docker image is used for each Taskcluster shard I don't know. But it'd be very good if it's also recorded in the wptreport somehow. (A bit backwards, I know, but a variable could be passed in.) |
Sorry I was busy (travelling) last week and not available on IRC. I've glanced through the PR. Apart from the nit comment I made earlier, I have one high-level question: is there no way to configure the number of chunks other than using a script to dump each chunk as a task? I'm a bit surprised if Taskcluster doesn't some sharding this natively. |
So. Tthe good way to support that is to write a decision task that schedules all the other tasks dynamically. We should do that, but it's not totally necessary for the first iteration. It's possible that there's some syntax for parameterising the yaml file; I can look for documentation. I'm not very pleased with the current setup either, but it seemed like the fastest path to something that worked. |
https://bugzilla.mozilla.org/show_bug.cgi?id=1335915 seems to be the bug for enabling the fancy templating stuff in |
This is a docker image based on the Ubuntu1604 base, with the runtime deps of Firefox and Chrome installed. There is also a shallow clone of wpt to act as a placeholder. The start.sh script will update this clone and start Xvfb, so it's possible to run tests.
TaskCluster is Mozilla's CI system, which also provides a GitHub integration. With this configuration enabling the TaskCluster integration on GitHub will cause each pust to master to create a TaskCluster run consisting of the full testsuite in latest Firefox Nightly and latest Chrome dev. The run will be chunked into 12 pieces for testharness.js tests and 6 for reftests, plus one for wdspec tests. With this chunking the end to end time for completing the run is around an hour (the task timeout is set to 1.5 hours). The resulting tasks upload wptreport.json files that can be used to reconstruct the results of the full run. There are also tbpl log files that could be used with e.g. Mozilla's reftest analyzer to inpect reftest failures.
This isn't very useful unless there is expectation data avaiable, which is not the common case
Also add a comment to indicate that the tasks file is generated.
4f87b52
to
e7bae7a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except for a nit.
(I agree with using a script to generate .taskcluster.yml for now. We can explore simplifications later.)
# to know that chrome requires | ||
|
||
RUN apt-get -qqy install \ | ||
firefox \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I meant everything below could be removed, i.e. just apt-get -qqy install firefox
should be enough.
The other packages below are "some extra deps we happen to know that chrome requires" according to the comment above. And since gdebi chrome.deb
will take care of them, we don't need to guess and install these dependencies any more.
The comment above should also be updated to reflect that we are only installing firefox now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Installing more stuff at build time does have the advantage that we don't do so much work at runtime, so generally it makes more sense to move stuff into the docker build rather than the other way around.
In this case we agreed on irc that it doesn't make much difference either way and there are plenty of performance optimisations and potential cleanup left on the table, so it's OK to just merging what we have now.
deb_archive=google-chrome-unstable_current_amd64.deb | ||
wget https://dl.google.com/linux/direct/$deb_archive | ||
|
||
sudo gdebi -n $deb_archive |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: Installing Google Chrome will add the Google repository so your system
will automatically keep Google Chrome up to date. If you don’t want Google's
repository, dosudo touch /etc/default/google-chrome
before installing the
package.
Source: https://www.google.com/intl/en/chrome/browser/
Is this relevant given the expected lifespan of the container?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No; any changes only persist for the lifetime of the container, which is just for a single test run.
Awesome! Looks like the very first run on master is this one, still ongoing: |
And here are actual results from shards from a shard that has finished: This will be great :) |
This PR actually has two parts:
wptreport.json
format; these could be combined to give the full set of results for each push.This change is