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.

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

Jump to bottom

[wptrunner] Re-enable return code #10742

Merged
merged 3 commits into from
May 9, 2018
Merged

[wptrunner] Re-enable return code #10742

merged 3 commits into from
May 9, 2018

Conversation

jugglinmike
Copy link
Contributor

@jugglinmike jugglinmike commented May 1, 2018 •
edited by wpt-reviewable-bot
Loading

In a previous commit [1] the WPT CLI was modified to ignore the return
value of the wptrunner.start method. Although this behavior is
convenient in cases where there is no expectation data, it obscures
errors that are relevant in any context.

Specifically, the start method returns a non-zero value when no tests
are run, but this is a reliable indicator of an erroneous configuration.
Because the wpt run command is used to validate the infrastructure of
the web-platform-tests project, ignoring this case allows
mis-configurations to go unnoticed [2].

Re-enable observance of the value returned by wptrunner.start and
update the code generated for the TaskCluster service to ignore this
value.

[1] 5a1b036
[2] #10721

This change is Reviewable

Hexcles
Hexcles reviewed May 1, 2018
View reviewed changes
tools/ci/taskgraph.py Outdated
%(command)s"""],
set -e
~/start.sh
(
Copy link
Member

Choose a reason for hiding this comment

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

Why is a subshell needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question. It isn't!

@Hexcles
Copy link
Member

Hexcles commented May 1, 2018 

 1:29.49 TEST_START: /infrastructure/browsers/firefox/prefs.html
 1:29.50 INFO Setting pref browser.display.foreground_color ('#00FF00')
 1:39.60 pid:6627 JavaScript error: executormarionette.py, line 84: TypeError: window.win.timeout is not a function
 1:44.66 TEST_END: TIMEOUT, expected OK

Hmm, weird.

@jugglinmike
Copy link
Contributor Author

That is strange. I'll investigate tomorrow

@wpt-pr-bot wpt-pr-bot requested a review from kereliuk May 2, 2018 19:53
@jugglinmike
Copy link
Contributor Author

It turns out that test has been timing out since its introduction (see this build report, for example), but we haven't been aware of it because we've been ignoring the status code. I've rebased this branch and added a separate commit to fix that bug (the original version of this branch is available here).

Hexcles
Hexcles approved these changes May 2, 2018
View reviewed changes
Copy link
Member

@Hexcles Hexcles left a comment

Choose a reason for hiding this comment

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

Yikes... Thanks for fixing this!

@gsnedders
Copy link
Member

Can you add a test in tools/wpt/tests for this? At least the "exits with non-zero when there are no matching tests" case should be easy enough.

jgraham
jgraham requested changes May 3, 2018
View reviewed changes
tools/ci/taskgraph.py Outdated
git checkout {{event.head.sha}}

# Test failures are not relevant in this context.
%(command)s || true
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't love this. I guess I'd prefer a --[no-]fail-on-unexpected command line option, which would have the advantage that we would also fails TC jobs if no tests are run.

@jgraham
Copy link
Contributor

jgraham commented May 3, 2018

So I apparently forgot to press "submit" on my review. Sorry. The gist is that we should make it possible to return success when there are unexpected results without making it necessary to ignore errors entirely. I think both my original patch and this one address the problem in the wrong layer and we should fix wptrunner to work the way we want (e.g. with another command line flag).

@jugglinmike jugglinmike mentioned this pull request May 3, 2018
Open
@jugglinmike jugglinmike force-pushed the wpt-fail-on-error branch 2 times, most recently from 03737c7 to f04ce3e Compare May 5, 2018 01:28
@jugglinmike
Copy link
Contributor Author

Thanks for the feedback @jgraham. I've implemented the new feature as part of this pull request.

Recent changes in master introduced merge conflicts in this patch, so I had to rebase this branch in order for it to apply cleanly. The original version of the branch is available here in case that's of interest to any of you folks.

The text stating that the new tests "don't work on Windows for path reasons" is copied from the surrounding tests; the tests are failing for me in Windows, and I presume that this is the same issue.

jgraham
jgraham approved these changes May 9, 2018
View reviewed changes
tools/ci/ci_taskcluster.sh Outdated
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
./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
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't want to do a drive-by fixup of the == that somehow got landed here despite (your?) earlier review comment pointing it out, do you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I sure do

@jugglinmike
[infra] Correct integration test
8271378 
By default, "single-page tests" must include an explicit call to the
global `done` function [1]. Insert this invocation to signal completion
and avoid a test timeout when the test's expectation is satisfied.

[1] http://web-platform-tests.org/writing-tests/testharness-api.html
@jugglinmike
[ci] Correct syntax in build script
041e0ed
@jugglinmike
[wptrunner] Allow test failures to be ignored
597050f 
In a previous commit [1] the WPT CLI was modified to unconditionally
ignore the return value of the `wptrunner.start` method. Although this
behavior is convenient in cases where there is no expectation data, it
obscures errors that are relevant in any context.

Specifically, the `start` method returns a non-zero value when no tests
are run, but this is a reliable indicator of an erroneous configuration.
Because the `wpt run` command is used to validate the infrastructure of
the web-platform-tests project, ignoring this case allows faulty
configurations to go unnoticed [2].

Implement a new CLI option named `--no-fail-on-unexpected` to allow
users to allow test failure while still being alerted to errors that
cause zero tests to be executed. Update the script designed for the
TaskCluster service to enable this option.

[1] 5a1b036
[2] web-platform-tests#10721
@jgraham jgraham merged commit f7a0aa0 into web-platform-tests:master May 9, 2018
@zcorpan zcorpan deleted the wpt-fail-on-error branch May 14, 2018 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgraham jgraham jgraham approved these changes

@Hexcles Hexcles Hexcles approved these changes

@gsnedders gsnedders Awaiting requested review from gsnedders

@kereliuk kereliuk Awaiting requested review from kereliuk

Assignees
No one assigned
Labels
infra
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jugglinmike @Hexcles @gsnedders @jgraham