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

Unify conditional test logic #6314

Merged
merged 1 commit into from
May 27, 2018
Merged

Unify conditional test logic #6314

merged 1 commit into from
May 27, 2018

Conversation

captbaritone
Copy link
Contributor

This makes the logic a bit easier to read, but also lets us skip some
integration tests on Jasmine, which will be needed as we make Circus
stricter.

@aaronabramov @rickhanlonii

@aaronabramov
Copy link
Contributor

we should also do skipOnNode([7, 8]) later!

@SimenB
Copy link
Member

SimenB commented May 27, 2018

Nice!

I've all but 3 integration tests pass on windows in #6310, can we merge that first?

@codecov-io
Copy link

codecov-io commented May 27, 2018

Codecov Report

Merging #6314 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6314   +/-   ##
=======================================
  Coverage   63.64%   63.64%           
=======================================
  Files         227      227           
  Lines        8638     8638           
  Branches        3        3           
=======================================
  Hits         5498     5498           
  Misses       3139     3139           
  Partials        1        1

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 d5dd58c...eaa1555. Read the comment docs.

@captbaritone
Copy link
Contributor Author

@SimenB Fine by me. I'm happy to rebase.

@SimenB
Copy link
Member

SimenB commented May 27, 2018

Awesome, I'll merge as soon as appveyor is green (any minute now)

@SimenB
Copy link
Member

SimenB commented May 27, 2018

Done! (and oh god, what a conflict list!)

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

Great stuff

This makes the logic a bit easier to read, but also lets us skip some
integration tests on Jasmine, which will be needed as we make Circus
stricter.
@thymikee
Copy link
Collaborator

Waiting with #6315 until this PR is merged :)

@captbaritone
Copy link
Contributor Author

Thanks @thymikee. Just waiting on CI. Will you merge for me once/if it passes?

@thymikee thymikee merged commit a43fd6c into jestjs:master May 27, 2018
@captbaritone
Copy link
Contributor Author

💚

thymikee added a commit to thymikee/jest that referenced this pull request May 27, 2018
* upstream/master:
  Unify conditional test logic (jestjs#6314)
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants