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

Do not run suite lifecycle methods when all descendant tests are skipped #961

Merged
merged 4 commits into from
Jan 2, 2019

Conversation

jonnycornwell
Copy link
Contributor

@jonnycornwell jonnycornwell commented Dec 2, 2018

  • Add tests to ensure suite lifecycle functions do not run if all tests are skipped
  • Evaluate grep up front rather than when the tests are executed

In our functional end-to-end tests we have had to write our own grep due to this, some of our before functions have substantial setup by navigating the app. Also mentioned in comment #950 (comment).

I have gone round the houses a bit with regards to this PR and think the dust has now settled, I was getting caught out by the fact grep is set on the rootSuite after instantiation, in that case it needs to process all the children recursively.

With regards to #950 (comment), I believe the tests should be added and skipped rather than just excluded.

p.s. This PR will have a merge conflict with my other PR (#955) relating to the type information for LifecycleMethod.

@codecov-io
Copy link

codecov-io commented Dec 2, 2018

Codecov Report

Merging #961 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #961      +/-   ##
==========================================
+ Coverage   92.64%   92.67%   +0.03%     
==========================================
  Files          58       58              
  Lines        4582     4601      +19     
  Branches      997     1000       +3     
==========================================
+ Hits         4245     4264      +19     
  Misses        337      337
Impacted Files Coverage Δ
src/lib/Suite.ts 96.67% <100%> (+0.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b99fe83...3a00d61. Read the comment docs.

@jonnycornwell
Copy link
Contributor Author

Just thought of a slight edge case related to my changes and have fixed accordingly

- Add tests to ensure suite/test life cycle functions do not run if all tests are skipped
- Evaluate grep up front rather than when the test is executed
@jonnycornwell jonnycornwell force-pushed the suite-lifecycle-updates branch from 0cc51aa to a0f21ec Compare December 2, 2018 21:25
- See line 798/799 in Executor
- Add associated test
@jonnycornwell jonnycornwell force-pushed the suite-lifecycle-updates branch from 0703e16 to 3a00d61 Compare December 3, 2018 13:30
@jonnycornwell
Copy link
Contributor Author

Upon investigating #962, I noticed that there was a race condition in the name being set after the grep following the other changes in this PR.

I have now added a failing tests for this and also a fix in 3a00d61

this._rootSuite.grep = grep;
this._rootSuite.name = name;
.

I am 99% sure I have no more changes or PRs to raise and no more changes to make to this PR!

@jason0x43
Copy link
Member

Ooo, this will be handy. I'll try to get to this in the relatively near future.

@jonnycornwell
Copy link
Contributor Author

jonnycornwell commented Dec 6, 2018

I am glad I left myself 1% wiggle room in my confidence. I have rolled this PR out throughout our builds and have found an issue with running unit tests via the Node runner. I haven't come across any issues when running functionalSuites or tests in the browser with the Html reporter.

Just trying to find the appropriate fix and will add it to this PR once I get a clean run through our builds

@jason0x43 jason0x43 merged commit 563680e into theintern:master Jan 2, 2019
jason0x43 pushed a commit that referenced this pull request Jan 11, 2019
…ped (#961)

* Do not run suite lifecycle methods when all descendant tests are skipped

- Add tests to ensure suite/test life cycle functions do not run if all tests are skipped
- Evaluate grep up front rather than when the test is executed

* Add test + fix edge case

* Add fix for race condition when name is set after grep

- See line 798/799 in Executor
- Add associated test

* Always run lifecycle functions for root suites
@jonnycornwell jonnycornwell deleted the suite-lifecycle-updates branch December 17, 2019 19:38
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.

3 participants