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

Add test for loading the various wptrunner products #9496

Merged
merged 3 commits into from Mar 2, 2018

Conversation

Projects
None yet
4 participants
@gsnedders
Copy link
Contributor

gsnedders commented Feb 13, 2018

These are tests to make sure we don't regress #9476. Note, however, they fail because of #9476 (review) (now addressed in second commit).

cc/ @jdm


This change is Reviewable

@wpt-pr-bot wpt-pr-bot added the infra label Feb 13, 2018

@wpt-pr-bot wpt-pr-bot requested a review from jgraham Feb 13, 2018

@gsnedders

This comment has been minimized.

Copy link
Contributor Author

gsnedders commented Feb 13, 2018

Hmm, both jobs passing on CI, that's a bad sign.

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Feb 13, 2018

Build PASSED

Started: 2018-03-02 17:09:12
Finished: 2018-03-02 17:22:58

View more information about this build on:

@gsnedders gsnedders force-pushed the gsnedders:load_products_tests branch 2 times, most recently from 72ea252 to 3129659 Feb 13, 2018

@gsnedders

This comment has been minimized.

Copy link
Contributor Author

gsnedders commented Feb 13, 2018

OK, fixed this to actually run the tox envs specified on Travis, so this now works.

@gsnedders gsnedders requested a review from jdm Feb 14, 2018

@gsnedders gsnedders added the wptrunner label Feb 14, 2018

@jgraham
Copy link
Contributor

jgraham left a comment

Seems OK I guess. I see the point given we missed some regressions here, although the generally crazy import stuff seems to make the tests a bit delicate.

The tox stuff really needs documentation because it's far from obvious what you're doing or why.

@@ -5,21 +5,30 @@ SCRIPT_DIR=$(dirname $(readlink -f "$0"))
WPT_ROOT=$(readlink -f $SCRIPT_DIR/../..)
cd $WPT_ROOT

run_applicable_tox () {

This comment has been minimized.

Copy link
@jgraham

jgraham Feb 15, 2018

Contributor

It is very unclear what this is for.

This comment has been minimized.

Copy link
@gsnedders

gsnedders Feb 15, 2018

Author Contributor

Does this comment address that?

products.load_product({}, product)
except ImportError as e:
if (current_tox_env and
product_env_map.get(product, product) in current_tox_env_split):

This comment has been minimized.

Copy link
@jgraham

jgraham Feb 15, 2018

Contributor

This indent is weird.

@gsnedders gsnedders force-pushed the gsnedders:load_products_tests branch from 3129659 to e452edb Feb 15, 2018

@gsnedders

This comment has been minimized.

Copy link
Contributor Author

gsnedders commented Feb 21, 2018

@jgraham ping

@jgraham

This comment has been minimized.

Copy link
Contributor

jgraham commented Feb 21, 2018

So per my comments on IRC, I don't really undrestand all the complexity here. I don't understand why there have to be three different environment variables or why we run all the tests but then ignore the results sometimes. I think this should either be simplified or more thoroughly commented to make it clear why this is the simplest possible approach.

@gsnedders

This comment has been minimized.

Copy link
Contributor Author

gsnedders commented Feb 21, 2018

@jgraham What three environment variables? The only environment variables here are TOXENV (pre-existing) and CURRENT_TOX_ENV (new).

I don't know how to explain the issue further…

Fundamentally the problem is that we have:

pnin:wpt gsnedders$ cd web-platform-tests/tools/
pnin:tools gsnedders$ tox -l
py27
py36
pypy
pnin:tools gsnedders$ cd wptrunner/
pnin:wptrunner gsnedders$ tox -l
py27-base
py27-chrome
py27-firefox
py27-sauce
py27-servo
pypy-base
pypy-chrome
pypy-firefox
pypy-sauce
pypy-servo
py27-flake8

We run both of these in the same Travis job, with the same TOXENV set to the installed Python version (so py27, py36, or pypy).

Except, as you can see, wptrunner doesn't define a tox environment with any of those names, but rather many of them with suffixes, and we want to run all of them (because e.g. py27-firefox can't load the Chrome product and py27-chrome can't load the Firefox product). Now, we could split up the Travis jobs and introduce a wptrunner_unittest or similar and then have all of those tox environment names listed in TOXENV variables in a job in .travis.yml, but that seems like even more complexity (and likely to be forgotten when we add a new environment).

As for running all the tests but ignoring the results sometimes, how do you suggest we do this? We probably shouldn't break running the tests outside of tox, but within tox we could potentially try to more aggressively skip them before running them for other jobs?

"servodriver": "servo",
}

@pytest.mark.parametrize("product", [

This comment has been minimized.

Copy link
@jgraham

jgraham Feb 21, 2018

Contributor

Right, so if we keep doing this like this, we should create a test helper that returns the list of active products or the full list, and parameterise over that.

This comment has been minimized.

Copy link
@gsnedders

gsnedders Mar 1, 2018

Author Contributor

I think in this case it actually makes sense, because we actually want to test something more: we want to check that it either that the product loads successfully or throws ImportError, and any other result should be a failure.

This comment has been minimized.

Copy link
@gsnedders

gsnedders Mar 1, 2018

Author Contributor

I've laid the groundwork to make this easy in future.

unset TOXENV
local RUN_ENVS=$(tox -l | grep "^${OLD_TOXENV}\(\-\|\$\)" | tr "\n" ",")
if [[ -n "$RUN_ENVS" ]]; then
tox -e "$RUN_ENVS"

This comment has been minimized.

Copy link
@jgraham

jgraham Feb 21, 2018

Contributor

So I'm not super happy about running every test N times even when the test doesn't do anything vaugely product-dependent.

This comment has been minimized.

Copy link
@gsnedders

gsnedders Mar 1, 2018

Author Contributor

I don't think that's worth optimising for at this stage when the whole testsuite takes under a second to run.

@gsnedders

This comment has been minimized.

Copy link
Contributor Author

gsnedders commented Mar 1, 2018

Note the CI failure here is #9725.

@jgraham

jgraham approved these changes Mar 2, 2018

@gsnedders gsnedders force-pushed the gsnedders:load_products_tests branch from ce47dda to f5913c5 Mar 2, 2018

@gsnedders gsnedders merged commit 00c4e79 into web-platform-tests:master Mar 2, 2018

1 check passed

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

@gsnedders gsnedders deleted the gsnedders:load_products_tests branch Mar 2, 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.