-
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 stable and beta releases on TaskCluster #12679
Conversation
@jugglinmike if you happen to already have a staging repo configured to run TaskCluster I would appreciate some testing of this branch. If you don't I can set one up myself. |
Also can we document how to set something up to test TC changes? |
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.
@jgraham in wpt.fyi, we use the terms "stable" and "experimental" to refer to
various browser releases generically. Internally, we map the latter to Chrome's
"dev" channel and Firefox's "nightly" channel. Taking that approach here would
allow us to expose a much more uniform interface. The new terminology will have
to be documented, but it may be easier to explain what we mean by
"experimental" versus having to interpret input like "Chrome nightly" or
"Firefox dev". What do you think?
@jugglinmike if you happen to already have a staging repo configured to run
TaskCluster I would appreciate some testing of this branch. If you don't I
can set one up myself.
Sure. I had to apply some of my suggestions to get it to run, but here's a push
to "epochs/daily": https://tools.taskcluster.net/groups/PS2oLaLRRZOl0M94VKHLZQ
tools/ci/ci_taskcluster.sh
Outdated
@@ -3,8 +3,8 @@ | |||
./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 | |||
./wpt run firefox --log-tbpl=../artifacts/log_tbpl.log --log-tbpl-level=info --log-wptreport=../artifacts/wpt_report.json --log-mach=- --this-chunk=$4 --total-chunks=$5 --test-type=$3 -y --install-browser --borwser-channel=$2 --no-pause --no-restart-on-unexpected --reftest-internal --install-fonts --no-fail-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.
Typo: --borwser-channel=$2
-> --browser-channel=$2
@@ -32,10 +33,21 @@ git checkout -b build ${REV} | |||
|
|||
sudo sh -c './wpt make-hosts-file >> /etc/hosts' | |||
|
|||
if [[ $BROWSER == "chrome"* ]] || [[ "$BROWSER" == all ]] | |||
if [[ $BROWSER == "chrome" ]] || [[ "$BROWSER" == all ]] |
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.
Appreciate the specificity this buys us
tools/docker/start.sh
Outdated
then | ||
deb_archive=google-chrome-stable_current_amd64.deb | ||
else | ||
exit 1 |
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.
While we're here, how about echo Unrecognized release channel: $CHANNEL >&2
?
tools/wpt/browser.py
Outdated
version = { | ||
"stable": "latest", | ||
"beta": "latest-beta", | ||
"nightly": "latest", |
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.
Mind removing the trailing comma for consistency?
tools/wpt/browser.py
Outdated
"""Install latest Browser Engine.""" | ||
if channel != "nightly": | ||
raise ValueError("Only nightly versions of Servo are availalbe") |
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.
Typo: "availalbe" -> "available"
tools/wpt/install.py
Outdated
def get_parser(): | ||
parser = argparse.ArgumentParser() | ||
parser.add_argument('browser', choices=['firefox', 'chrome'], | ||
parser.add_argument('browser', choices=['firefox', 'chrome', 'servo'], |
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.
Good idea
tools/wpt/install.py
Outdated
@@ -16,6 +19,13 @@ def get_parser(): | |||
def run(venv, **kwargs): | |||
browser = kwargs["browser"] | |||
destination = kwargs["destination"] | |||
channel = kwargs["channel"] | |||
if browser == "chrome" and channel == "nightly": |
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 don't think many users will expect this behavior. Would you be open to failing under these conditions instead?
.taskcluster.yml
Outdated
$map: | ||
$if: event.ref == 'refs/heads/master' | ||
then: | ||
{config: [{name: firefox, channel: nightly}, {name: chrome, channel: dev}]} |
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.
We'll need to remove {config: ...}
and render the array values directly
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.
Separately, the $match
operator might make this section more readable:
$if: tasks_for == "github-push"
then:
$map:
$flatten:
$match: {
event.ref == "refs/heads/master": [{name: firefox, channel: nightly}, {name: chrome, channel: dev}],
event.ref == "refs/heads/epochs/daily": [{name: firefox, channel: beta}, {name: chrome, channel: beta}],
event.ref == "refs/heads/epochs/weekly": [{name: firefox, channel: stable}, {name: chrome, channel: stable}]
}
each(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.
I didn't do that because it's not exhaustive, but if
/else
is. I agree that $match
is more readable, but I don't know what the right tradeoff is. Maybe I shouldn't care about handling every case since hitting a case that isn't handled here resulting in a runtime error would be fine.
tools/wpt/browser.py
Outdated
"nightly": "latest", | ||
} | ||
if channel not in branch: | ||
raise ValueError |
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.
Same as above regarding error message
The
I'm open to suggestion for improving this. Do you think more of an explanation |
Regarding using experimental/stable, I don't think that quite works because we have three states; nightly, beta and stable. In fact for Chrome there are four states; canary, dev, beta and stable. Running stable and beta gives us the ability to test version N+1 and version N simultaneously (assuming that beta and stable are both using the same configuration which is more or less true for gecko at least). I think that latest/beta/stable would be a sufficient ontology to cover our needs (although I'm not exactly sure how Edge and WebKit/Safari fit in there). In terms of actual command line arguments, if we expect people to run this generally I would prefer to be liberal in what we accept rather than forcing people to learn a specific set of nomenclature (e.g. I would happily accept all of experimetal, nightly, dev, preview, latest to mean whatever the most up to date version of the browser is (dev is perhaps the least appealing since Firefox has a developer edition that isn't quite the same thing). And equally I would happily accept stable, release, and other synonyms used for the stable channel. |
https://tools.taskcluster.net/groups/KkY09Cj_QKO4A2JPHUyn_Q is a run starting from a `epochs/daily' push on my fork. |
Alright, so first of all the installation part looks good to me. We don't currently have the infra for pushing Lastly, while you're at it, could you add |
@foolip created some infrastructure for pushing to the |
.taskcluster.yml
Outdated
@@ -4,10 +4,16 @@ policy: | |||
tasks: | |||
$if: tasks_for == "github-push" | |||
then: | |||
$if: event.ref == "refs/heads/master" | |||
$if: event.ref == "refs/heads/master" || event.ref == "refs/heads/epochs/daily" || event.ref == "refs/heads/epochs/weekly" |
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.
With the introduction of $match
, this condition is no longer necessary.
Submitting the task to Taskcluster failed. Detailsbad indentation of a mapping entry at line 15, column 13: |
4169de4
to
e30043c
Compare
https://tools.taskcluster.net/groups/NTBwJqX_TNiRKL-FEPegPw is a run of this. |
Exciting! I guess that wasn't triggered by a branch update, but otherwise is the same as what we want? |
It wasn't but I can do that too if you like. I had an earlier run that was triggered by a branch update so I'm pretty sure that works, I was mostly checking my refactoring didn't break the yml file entirely. |
The channel derived from the binary turns out to not always be reliable. Also we were using an incorrect tagging scheme for beta prefs, so we tried to download a file that didn't exist. To prevent clashes store downloaded firefox and its prefs in channel-specific directories.
@jugglinmike Added some more help text. |
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.
Just got one question regarding the --browser-version
argument.
version, channel_ = self.get_version_and_channel(binary) | ||
if channel is not None and channel != channel_: | ||
# Beta doesn't always seem to have the b in the version string, so allow the | ||
# manually supplied value to override the one from the binary |
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.
Wow, really? This is good to know. It's yet another reason why we need to include channels in wptreport.json
. Currently the receiver infers the channel from the version string, which according to this comment is apparently unreliable.
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 only found this out when writing this patch :) I think what happens is that shortly before release the beta switches to the actual release config and so gets a release-like version string. But I"m not sure.
'nightly': latest_channels, | ||
'dev': latest_channels, | ||
'preview': latest_channels, | ||
'experimental': latest_channels, |
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 suppose "preview" might be used for the future Safari support, perhaps?
@@ -192,6 +192,12 @@ def create_parser(product_choices=None): | |||
help="Path to directory containing extra json files to add to run info") | |||
config_group.add_argument("--product", action="store", choices=product_choices, | |||
default=None, help="Browser against which to run tests") | |||
config_group.add_argument("--browser-version", action="store", | |||
default=None, help="Informative string detailing the browser " | |||
"release version. This is included in the run_info data.") |
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 my earlier change, I added this to wpt/run.py
:
Line 439 in 1de1550
kwargs['browser_version'] = setup_cls.browser.version(kwargs.get("binary")) |
Would it always override the command line argument?
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.
Yes. But we can remove that in a future patch.
This (ab)uses mozdownload to grab the common.tests.tar.gz package from the latest nightly build and extracts geckodriver from that. The main disadvantage is that it's rather slow since the package itself is rather large and we want only a small part of it. The solution to this would be to package up geckodriver by itself on taskcluster, but that will require changes in the gecko buildsystem.
Install nightly geckodriver when testing Firefox nightly.
This change makes the results processor to save the merged report instead of just the last chunk of the report if the original report is chunked when uploaded. Besides, it also sets the channel label (stable/experimental) based on a new property in the report, `run_info.browser_channel`, introduced in web-platform-tests/wpt#12679 .
Oh cool, this is merged! epochs/daily currently points to 9a5d71b and I see that we had beta results: However, I can't find the regular (experimental) results. Did beta run instead of experimental? |
$map: | ||
$flatten: | ||
$match: { | ||
event.ref == "refs/heads/master": [{name: firefox, channel: nightly}, {name: chrome, channel: dev}], |
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.
The answer to my question is probably here, but I can't interpret it. Pushes to master and pushes to epochs/* will be separate events, but they will be the same sha. Does Taskcluster do something to avoid retesting the same sha? Or is it perhaps just task group triggered by push to master is now hidden in GitHub's UI? @Hexcles, if that is the case, will your integration code be able to find both after the fact, or will it only work if we react to the webhooks as the runs are happening?
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.
The answer is actually at https://api.github.com/repos/web-platform-tests/wpt/commits/9a5d71b326166e12784bdd9d161772e20f87c1fd/statuses There are two success
statuses with context Taskcluster (push)
, one with the URL https://tools.taskcluster.net/groups/dMjbBGL-TPu9z6TuZYzonw and one with the URL https://tools.taskcluster.net/groups/FFjitCXpQ2C4fD_7O7gPeA
GitHub groups by context so it only displays one run in the UI. AFAIK there isn't a way to have the TaskCluster tasks get different GitHub context values (possibly we could add that feature?) so we can still get at all the results even thought we can't seee them all.
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.
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.
My code listens to the "status" webhook events, whose payload is similar to that API. There'll be two separate updates in this scenario and my code will run twice. Therefore, all Taskcluster runs will be uploaded as long as the status is "success", regardless of how GitHub UI displays them and whether they are on the same revision.
When the uploader does not provide the channel label (stable/experimental), the processor will now set it based on a new property in the report, `run_info.browser_channel`, introduced in web-platform-tests/wpt#12679 .
When the uploader does not provide a channel label, the processor will now set it based on a new property in the report, `run_info.browser_channel`, introduced in web-platform-tests/wpt#12679 . A new channel label, "beta", is also introduced in this commit, since we now have beta browsers running on Taskcluster daily.
@jgraham are we creating artifacts especially for the logs? If not yet, what is the follow-up issue to get this integrated? |
Log files are available for all builds. They can be downloaded via the TaskCluster UI or via the command |
@jugglinmike I tried but I cannot find any log for https://tools.taskcluster.net/groups/ccZFTpXgQ1CqBIHft81eXQ which was triggered by #12968. Taskcluster doesn't list any artifacts nor logs. |
Oh, wait. That is actually a task group! Navigating to a single test job I can see the artifacts now. So all is fine. |
@whimboo https://tools.taskcluster.net/groups/ccZFTpXgQ1CqBIHft81eXQ/tasks/DuJQTji0QomkUiTD3q3CCg/runs/0/logs/public%2Flogs%2Flive.log You need to select a task in the group first. Then you'll see some logs in the "Run Logs" dropdown. By the way, this particular example had no "affected tests", so no tests ran. |
When the uploader does not provide a channel label, the processor will now set it based on a new property in the report, `run_info.browser_channel`, introduced in web-platform-tests/wpt#12679 . A new channel label, "beta", is also introduced in this commit, since we now have beta browsers running on Taskcluster daily.
No description provided.