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

Fix runs beforeAll/afterAll in excluded suites (#4820) #6234

Merged
merged 2 commits into from
May 30, 2018
Merged

Fix runs beforeAll/afterAll in excluded suites (#4820) #6234

merged 2 commits into from
May 30, 2018

Conversation

jbdemonte
Copy link
Contributor

Disable run of beforeAll when test suite is desactivated

Summary

Test plan

describe('ROOT', () => {
  describe('SUB_1', () => {
    beforeAll(() => {
      console.log('SUB_1: beforeAll')
    });
    it('test 1', () => {
      console.log('SUB_1: test 1');
    });
  });

  describe('SUB_2', () => {
    beforeAll(() => {
      console.log('SUB_2: beforeAll')
    });
    it('test 2', () => {
      console.log('SUB_2: test 2');
    });
  });
});

run using jest test.js --testNamePattern="test 1"

Logs before the modification:

 PASS  ./test.js
  ROOT
    SUB_1
      ✓ test 1 (1ms)
    SUB_2
      ○ skipped 1 test

  console.log test.js:4
    SUB_1: beforeAll

  console.log test.js:7
    SUB_1: test 1

  console.log test.js:14
    SUB_2: beforeAll

Logs after the modification:

 PASS  ./test.js
  ROOT
    SUB_1
      ✓ test 1 (1ms)

  console.log test.js:4
    SUB_1: beforeAll

  console.log test.js:7
    SUB_1: test 1

@SimenB
Copy link
Member

SimenB commented May 23, 2018

This is awesome, thanks for tackling it!

Mind adding an integration test?

CHANGELOG.md Outdated
@@ -168,6 +168,8 @@
regardless of order ([#6150](https://github.com/facebook/jest/pull/6150))
* `[pretty-format]` [**BREAKING**] Remove undefined props from React elements
([#6162](https://github.com/facebook/jest/pull/6162))
* `[jest-cli]` Fix run beforeAll in excluded suites
tests" mode. ([#4820](https://github.com/facebook/jest/issues/4820))
Copy link
Member

Choose a reason for hiding this comment

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

should point to the PR, not the issue 🙂

@jbdemonte
Copy link
Contributor Author

I wanted to add an integration test, but I didn't find how to, and I had no time to dig how those tests worked, I'll try to take the time by the end of the week

@jbdemonte
Copy link
Contributor Author

@SimenB Sorry, but I really have no idea of where / how to test it, right now, I still have not undestood how tests works in jest :(

@SimenB
Copy link
Member

SimenB commented May 29, 2018

All the directories in https://github.com/facebook/jest/tree/master/e2e are integration tests. They each have a corresponding test in https://github.com/facebook/jest/tree/master/e2e/__tests__ which spawns up a jest instance.

You can look at e.g, https://github.com/facebook/jest/tree/master/e2e/before-each-queue for an example of a test with hooks and assertions on the output.


@rickhanlonii any progress on that contributing guide? :)

@jbdemonte
Copy link
Contributor Author

arf, I was trying to play in the folder /Users/jbd/git/reboot/jest/packages/jest-jasmine2/src/__tests__

@jbdemonte
Copy link
Contributor Author

I'm confused, it seems to work on my machine (as usual) but fails on the CI

@SimenB
Copy link
Member

SimenB commented May 29, 2018

We're in the process of replacing jasmine as the underlying runner with jest-circus (#6295).

If you run JEST_CIRCUS=1 ./jest before-all-filtered.js you should see the failure locally as well 🙂

@codecov-io
Copy link

Codecov Report

Merging #6234 into master will decrease coverage by 0.07%.
The diff coverage is 5.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6234      +/-   ##
==========================================
- Coverage   63.89%   63.81%   -0.08%     
==========================================
  Files         228      228              
  Lines        8705     8717      +12     
  Branches        4        4              
==========================================
+ Hits         5562     5563       +1     
- Misses       3142     3153      +11     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-jasmine2/src/tree_processor.js 0% <0%> (ø) ⬆️
packages/jest-circus/src/utils.js 21.18% <9.09%> (-0.06%) ⬇️

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 86c91d5...996e2a9. Read the comment docs.

@jbdemonte
Copy link
Contributor Author

I had no clue there were two different runner :)

@thymikee
Copy link
Collaborator

It's quite fresh, don't tell anyone 😉

@SimenB
Copy link
Member

SimenB commented May 29, 2018

@aaronabramov mind taking a quick look at the circus implementation?

@jbdemonte
Copy link
Contributor Author

just rebased

@aaronabramov
Copy link
Contributor

wait.. it should already work in jest-circus! i remember i worked on it last year! :)

@aaronabramov
Copy link
Contributor

ah yeah.. i see! we do still run hooks even if it's skipped.
that fix looks great!

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants