Skip to content

Give all Travis jobs human-readable names#13300

Merged
foolip merged 6 commits intomasterfrom
foolip/travis-job-names
Oct 2, 2018
Merged

Give all Travis jobs human-readable names#13300
foolip merged 6 commits intomasterfrom
foolip/travis-job-names

Conversation

@foolip
Copy link
Member

@foolip foolip commented Oct 1, 2018

Also make travis.yml more consistent around os and python.

Also make travis.yml more consistent around `os` and `python`.
@wpt-pr-bot wpt-pr-bot requested a review from jgraham October 1, 2018 21:15
@foolip
Copy link
Member Author

foolip commented Oct 1, 2018

I did this because I've been unable to understand what some of the jobs are from a view like https://travis-ci.org/web-platform-tests/wpt/builds/435826707.

https://travis-ci.org/web-platform-tests/wpt/builds/435843332 shows what this would look like.

.travis.yml Outdated
fast_finish: true
include:
- os: linux
- name: "manifest upload"
Copy link
Member Author

Choose a reason for hiding this comment

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

Should I add "(no-op for PR)" or something to this description?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll go with that.

.travis.yml Outdated
- libnss3-tools
env: JOB=wpt_integration TOXENV=py27,py27-flake8 SCRIPT=tools/ci/ci_wpt.sh
- os: linux
- name: "wptrunner infrastructure"
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this a good name? Are these unit tests, or what?

Copy link
Contributor

Choose a reason for hiding this comment

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

This runs pytest in tools/wpt

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, that doesn't seem quite right. The only place pytest appears in tools/ci is in tools/ci/tests/test_jobs.py, and this job will run tools/ci/ci_wptrunner_infrastructure.sh. Looks like it will run infrastructure/ in chrome and firefox, using in-tree expectations. Am I reading that right?

If so, maybe call it "infrastructure/ tests"? I'll throw on a slash on some of the other names too for consistency.

Copy link
Contributor

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

Oh, I didn't know about this.

@foolip foolip requested a review from gsnedders October 2, 2018 14:08
.travis.yml Outdated
python: "2.7"
env: JOB=resources_unittest TOXENV=py27 SCRIPT=tools/ci/ci_resources_unittest.sh
- python: 2.7
- name: "flake8"
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm now pretty sure I've misnamed this one. This does involve tools/wpt and is probably what @jgraham referred to in https://github.com/web-platform-tests/wpt/pull/13300/files#r221878898. This does run flake8, but also other things it seems. @gsnedders, what should it be called? "tools/wpt/ pytest + flake8"?

Copy link
Contributor

Choose a reason for hiding this comment

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

"wpt unittests", for consistency with all the other unittests jobs? (Or "wpi cli unittests", if that's unclear?)

Copy link
Member

Choose a reason for hiding this comment

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

"wpt cli unittests" would be much clearer IMO

Copy link
Member

Choose a reason for hiding this comment

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

Or even tools/wpt/ unittests to be consistent with the above?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll go with tools/wpt/ unittests, if they are indeed accurate described as unit tests.

@foolip
Copy link
Member Author

foolip commented Oct 2, 2018

Does anyone know why we run flake8 the way we do, and not as part of the lint, say? Would it be reasonable to restructure the jobs to run flake8 as part of the lint job? (I'd just file a follow-up issue, not here.)

@jgraham
Copy link
Contributor

jgraham commented Oct 2, 2018

Making the lint just run wpt lint seems desirably simple; if you have to look at the output you don't have to search through for error messages but get something that looks a lot like the output you will get at the command line with all the errors at the bottom. Given that people changing tests are typically not that familiar with the project that seems desirable and putting additional complexity onto infra people who are heavily involved seems like a reasonable way to manage it.

@foolip
Copy link
Member Author

foolip commented Oct 2, 2018

So, it seems like we run flake8 as part of 3 tox.ini files, run in 2 different jobs:
tools/tox.ini (run by tools/ci/ci_tools_unittest.sh)
tools/wpt/tox.ini (run by ci_wpt.sh)
tools/wptrunner/tox.ini (run by tools/ci/ci_tools_unittest.sh)

That's kind of weird, but I'll just leave it alone in this PR.

However, poking around I noticed that "resources/ unittests" is not pedantically correct, as resources/test/tests/ has both functional and unit subdirectories. I'll just call it "resources/ tests".

@foolip
Copy link
Member Author

foolip commented Oct 2, 2018

OK, I've tinkered a bit more, changed the order to make it read better. Enough tinkering, now merging. Please shout at me if you don't like the names :)

@foolip foolip merged commit 00c4ce1 into master Oct 2, 2018
@foolip foolip deleted the foolip/travis-job-names branch October 2, 2018 20:43
@jugglinmike
Copy link
Contributor

This is great!

boazsender pushed a commit to bocoup/wpt that referenced this pull request Sep 10, 2019
Also make travis.yml more consistent about the use/quoting of
`os` and `python` and make order nicer given the new names.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants