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

Run only tests associated with updated example directory #1100

Merged
merged 6 commits into from
Aug 24, 2019

Conversation

spectranaut
Copy link
Contributor

This PR cuts down the number of tests run on each PR. It runs the tests by example directory, so all of the tests for all examples in an example directory will always be run at once. But if only on example directory is updated, then only the tests for that example directory will run.

The travis script I uploaded does the following:

  1. When there is an update to any file in the 'examples/' directories, it will run the all the tests of all examples in the directory. For example, if there is an update to examples/tabs/css, the tests for examples/tabs/tabs-1/tabs.html and examples/tabs/tabs-2/tabs.html will be run.
  2. When there is an update to a test file, it will run all the tests related to the example directory. For example, if only the tabs-1 test file is update, both tabs-1 and tabs-2` tests will run.
  3. If any code in examples/js or examples/css is edited, then all tests are run.
  4. If test/index.js or any code in test/util is edited, then all tests are run.

Tests are batched by directory in order to keep the travis script as simple as possible. In some cases, you can't tell which tests to run based on which files where edited.

Additionally, this PR also changes the name format for the test files. All test files must be prepended with the directory of the example they are testing in order to easily filter tests. If anyone has a strong objection to this naming convention, then I could look into doing something else -- such as replicating the examples directory structure under test/tests. I chose renaming the test file because it was the simplest solution and keeps the travis script simple as well.

Finally, this PR changes the format of the output from AVA's format to TAP. TAP is a standard where as AVA's format is not. I hope this will make it slightly easier to navigate. If there is a failure, you search for the TAP standard "NOT OK", and the failure will be detailed in the following lines.

@spectranaut
Copy link
Contributor Author

I added two commits:

@spectranaut
Copy link
Contributor Author

hey @nschonni could you review this? :)

Copy link
Contributor

@nschonni nschonni left a comment

Choose a reason for hiding this comment

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

LGTM.
One that i'm note sure is if it might be easier to mirror the example and test directory. Maybe that's a AVA pattern.
EX:

example\
  alert\alert.html
  accordion\accordion.html
test\
  alert\alert.js
  acordion\accordion.js

Then the affected files with replace "example" with "test" (and maybe html with js). This is just bikeshedding though 😄
Edit: I guess that regex replace in my way wouldn't be straight forward for the css/js subfolder changes

@spectranaut
Copy link
Contributor Author

@nschonni I think the directory structure copying would work fine, actually. I could write a regex for it and run the ava tests. I bike sheded myself for maybe too long on which to do ;D

@mcking65 is there anyone else who should take a look at this or weigh in?

@nschonni
Copy link
Contributor

nschonni commented Jul 26, 2019

Oh, thought of one thing that might be a problem

git:
depth: 3
only the last 3 commits are fetched. Might need to increase that a bit, or remove that optimization

@spectranaut
Copy link
Contributor Author

Wow I'm glad you caught this configuration.. but I also don't know if I understand the purpose/effect of it.. does this mean that git diff --name-only $TRAVIS_COMMIT_RANGE will only return the diff between at most the last three commits?

I can do a quick test to confirm, I think.

@nschonni
Copy link
Contributor

I think so, but I'm also not sure if it would fail, or silently fetch, or just blow up

@spectranaut
Copy link
Contributor Author

Ok so I did a test and it actually behaves the way we want, look at this PR on the bocoup fork: bocoup#18 I edited a test file, committed, then added three commits just editing the spec. The script successfully picks up the edited test file and runs only the tests for that test file:
https://travis-ci.org/bocoup/aria-practices/jobs/564157243

I printed the $TRAVIS_COMMIT_RANGE before running the tests here, and it does include al 5 commits:
https://travis-ci.org/bocoup/aria-practices/jobs/564157243#L262

@nschonni
Copy link
Contributor

cool, I guess it must pull down the commits if needed, or maybe it has to pull them down anyway

@@ -0,0 +1,36 @@
#!/bin/bash

if ! git diff --name-only $TRAVIS_COMMIT_RANGE | grep -qP '(test/|examples/|package\.json)'
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it also include "scripts" and ".travis.yml"?

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 don't think it should include "scripts", it would make it super hard to test changes to "scripts/regression-tests.sh", because this will always shortcut the script and run all tests.

Maybe the same thing for ".travis.yml"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just "scripts/reference-tables.*" since that would affect the tests potentially? or is that covered by the "example/" change already?
The travis.yml is probably also an edge case right now, that I hit with #1116 since it potentially affects regression tests, but didn't trigger

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah to test the .travis.yml chances I opened a lot of PRs on the bocoup branch. As for the "reference-tables" -- this produced an html page that isn't tested by the regression tests (because it has not examples on it), so it shouldn't trigger any tests I think!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PRs with test files or example files edited, to trigger the tests actually running, is what I mean, see here

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I guess you can always temporarily game the build by touching a test file if you want to trigger for .travis.yml changes

Copy link
Contributor

@mcking65 mcking65 left a comment

Choose a reason for hiding this comment

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

LGTM; Thank you @nschonni for all the feedback.

I am comfortable with the naming requirements now that we have them tested by the coverage report.

@mcking65 mcking65 merged commit d40a173 into w3c:master Aug 24, 2019
michael-n-cooper pushed a commit that referenced this pull request Aug 24, 2019
Regression Tests: Run only tests associated with updated example directory (pull #1100)

These changes Cut down the number of tests run on each PR.

* Tests are run by example directory, so all tests for all examples in an example directory will always be run at once.
* But if only one example directory is updated, then only the tests for that example directory will run.
* If any code in `examples/js` or `examples/css` is edited, then all tests are run.
* If `test/index.js` or any code in `test/util` is edited, then all tests are run.
* If package.json is updated, then all tests
are run.
* Changes the name format for the test files. All test files must be prepended with the directory of the example they are testing in order to easily filter tests.
* Test report will report bad test file names.
* Changes the format of the output from AVA's format to TAP.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants