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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a self-test skip option #9579

Merged
merged 5 commits into from Jan 29, 2018
Merged

Conversation

hwillson
Copy link
Contributor

While doing some test work on another PR, I accidentally ran a meteor self-test without my usual --exclude list. When that happened, I noticed that cc47278 broke one of the command-line: old cli tests (converted) self-test's, that is no longer usually called (it's currently excluded by the CI configs). Looking into fixing this has sent me on a complete tangent (it must be Friday 馃檪), leading to this PR ...

Meteor's CI infrastructure is configured to exclude certain self-test's on each run. These excludes are specified in each CI environment's config file, and included when running meteor self-test. Developers running meteor self-test locally however are not using these excludes by default, so developer's have to manually look up the current exclude list from one of the CI configs, then add these excludes to their own meteor self-test call manually.

This commit adds a new skip option to Meteor's self-test system, that can be used to skip adding/running a defined self-test (similar in concept to Mocha's skip feature). This provides a way to skip the running of older self-test's that are no longer needed, but allows them to be preserved in the self-test suite, for future reference. With this functionality in place, and the older test suites updated to use it, Meteor's base CI excludes no longer need to be maintained in their respective config files. The excludes are all managed at the source (the test definition), and can be leveraged by anyone/anything calling meteor self-test.

Thanks!

Meteor's CI infrastructure is configured to exclude certain
`self-test`'s on each run. These excludes are specified in
each CI environment's config file, and included when running
`meteor self-test`. Developers running `meteor self-test`
locally however are not using these excludes by default,
so developer's have to manually look up the current exclude
list from one of the CI configs, then add these excludes to
their own `meteor self-test` call manually.

This commit adds a new `skip` option to Meteor's `self-test`
system, that can be used to skip adding/running a defined
`self-test` (similar in concept to Mocha's `skip` feature).
This provides a way to skip the running of older
`self-test`'s that are no longer needed, but allows them to
be preserved in the `self-test` suite, for future reference.
With this functionality in place, and the older test suites
updated to use it, Meteor's base CI excludes no longer need
to be maintained in their respective config files. The
excludes are all managed at the source (the test definition),
and can be leveraged by anyone/anything calling
`meteor self-test`.
// e.g. `selftest.skip.define("some test", ...` will skip running "some test".
export const skip = {
define() {
/* no-op */
Copy link
Contributor

Choose a reason for hiding this comment

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

Sometimes test frameworks will print something to indicate that a test is being skipped. Would that be useful 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.

Thanks for the suggestion @benjamn - I originally logged a "Skipped" message, but decided to take it out when I saw how it looked alongside the final self-test summary message. I'll take another pass at it though - thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just as an example, this is how it looked.

screenshot 2018-01-26 20 22 41

Because skip is called before define, the message gets logged when all of the tests are first attempted to be defined. So even when using a self-test exclude pattern, all .skip configured tests are still dumped to the console. Maybe that's okay - I just thought it looked a bit odd ...

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've added a skipped test message that looks like this:

...
Ignoring "old cli tests (converted)" test (excluded by selftest.skip)
Ignoring "compiler plugins - addAssets" test (excluded by selftest.skip)
...

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 I was thinking we should only log the skips if those tests would have been run. If that's not easy, this isn't essential.

@hwillson
Copy link
Contributor Author

@benjamn I've pushed some tweaks to make this fall more in-line with the existing self-test summary. How about this:

screenshot 2018-01-26 21 37 56

So we don't show the name of the manually ignored tests, but we do list a manually ignored count (and mention that they were ignored by selftest.skip). This lines up with the approach used for the other skip tags, like "non-matching" - it doesn't show the names of the non-matching tests, just a count.

Let me know if this works - thanks!

Copy link
Contributor

@benjamn benjamn left a comment

Choose a reason for hiding this comment

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

This is great!

Copy link
Contributor

@abernix abernix left a comment

Choose a reason for hiding this comment

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

Just a couple questions, but this looks great to me, even without addressing them. Feel free to merge this once you've reviewed!

"
# If needed, for readability this should be a regex wrapped across
# multiple lines in quotes.
SELF_TEST_EXCLUDE: ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get rid of this completely now?

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 left it there in-case we wanted to add one-off excludes for specific CI configs, like maybe if there were some excludes we needed for Circle but not AppVeyor. Happy to remove them if you don't think this is needed though!

^minifiers: apps can't use|\
^compiler plugins - addAssets|\
^NULL-LEAVE-THIS-HERE-NULL$"
SELF_TEST_EXCLUDE: "^NULL-LEAVE-THIS-HERE-NULL$"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question about removing here.

(The ^NULL-LEAVE-THIS-HERE-NULL$ was, IIRC, to prevent odd command line behavior exhibited when no tests were defined).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above comment - I guessed that was the reason for NULL-LEAVE-THIS-HERE-NULL, so I left it in place. Thanks!

@abernix abernix merged commit 540dc00 into meteor:devel Jan 29, 2018
@abernix
Copy link
Contributor

abernix commented Jan 29, 2018

Works for me! Thanks for this great improvement! Now I can clear my local environment variables, of the same form, too. 馃槈

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