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

Exit check_stability with an error when no tests run #10098

Merged
merged 2 commits into from Mar 20, 2018

Conversation

Projects
None yet
4 participants
@Hexcles
Copy link
Member

Hexcles commented Mar 19, 2018

Similar to #10030 (which was for wpt run), this commit makes wpt check-stability return a non-zero exit code when no tests run.

Also fix some code smells:

  • run was redefined by do_delayed_imports to a different function.
    Rename the other run method in this module to setup_and_run.
  • retcode was unnecessarily defined in the top scope. Remove it to
    avoid redefinition.
  • global logger and the call to get_parser were extraneous in run
    (now setup_and_run) and hence are removed.

This change is Reviewable

Exit check_stability with an error when no tests run
Similar to #10030 (which was for `wpt run`), this commit makes `wpt
check-stability` return a non-zero exit code when no tests run.

Also fix some code smells:

* `run` was redefined by `do_delayed_imports` to a different function.
  Rename the other `run` method in this module to `setup_and_run`.
* `retcode` was unnecessarily defined in the top scope. Remove it to
  avoid redefinition.
* `global logger` and the call to `get_parser` were extraneous in `run`
  (now `setup_and_run`) and hence are removed.

@Hexcles Hexcles requested review from gsnedders and jgraham Mar 19, 2018

@wpt-pr-bot wpt-pr-bot added the infra label Mar 19, 2018

@Hexcles

This comment has been minimized.

Copy link
Member Author

Hexcles commented Mar 19, 2018

One more step towards #9932 .

@Hexcles

This comment has been minimized.

Copy link
Member Author

Hexcles commented Mar 19, 2018

One example: https://travis-ci.org/w3c/web-platform-tests/jobs/355393374

This job didn't run any tests but the was still green.

def run(venv, wpt_args, **kwargs):
global logger

def setup_and_run(venv, wpt_args, **kwargs):

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 19, 2018

Contributor

This will break the wpt check-stability command since it uses run as the entry point.

This comment has been minimized.

Copy link
@Hexcles

Hexcles Mar 20, 2018

Author Member

Renamed the other run to stability_run instead.

@@ -353,16 +350,15 @@ def run(venv, wpt_args, **kwargs):
status="failed" if inconsistent else "passed")
else:
logger.info("No tests run.")
retcode = 3

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 19, 2018

Contributor

Are we sure this never happens in practice? I'm slightly concerned that maybe we sometimes pass this a list of things that could be tests but actually aren't so nothing would actually run. I think it should be OK but there might be an edge case like a .py file under webdriver that doesn't have any test functions, or something.

This comment has been minimized.

Copy link
@Hexcles

Hexcles Mar 20, 2018

Author Member

No you're right. It's totally possible to have tests_changed empty but files_affected not, though I'm not sure if the scenario you described would be an edge case (not familiar with webdriver/*).

We should dive deeper: raise an exception from wptrunner.run_tests.

This comment has been minimized.

Copy link
@Hexcles

Hexcles Mar 20, 2018

Author Member

Sorry, going down into wptrunner.run_tests doesn't really solve the problem at all.

Let me be conservative and only return errors if tests_changed is non-empty.

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 19, 2018

Build PASSED

Started: 2018-03-20 18:54:33
Finished: 2018-03-20 19:12:24

View more information about this build on:

Address comments
* Renaming `check_stability.run` breaks commands.json and naming
  conventions, so rename the other `run` in that module instead.
* Only return errors for empty results when we know for sure tests are
  affected.
@jgraham
Copy link
Contributor

jgraham left a comment

Thanks!

@Hexcles Hexcles merged commit 3a83284 into master Mar 20, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Hexcles Hexcles deleted the fix-check-stability branch Mar 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.