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

Add each to jest-cirucs #6309

Merged
merged 14 commits into from
May 28, 2018
Merged

Add each to jest-cirucs #6309

merged 14 commits into from
May 28, 2018

Conversation

mattphillips
Copy link
Contributor

Summary

Add each to jest-cirucs

Test plan

  • Add new describe.only.each and describe.skip.each tests
  • Run tests with JEST_CIRCUS flag set and the tests should still pass

@@ -14,10 +14,8 @@ const runJest = require('../runJest');
const {extractSummary} = require('../Utils');
const dir = path.resolve(__dirname, '../each');
const SkipOnWindows = require('../../scripts/SkipOnWindows');
const SkipOnJestCircus = require('../../scripts/SkipOnJestCircus');

SkipOnWindows.suite();
Copy link
Collaborator

Choose a reason for hiding this comment

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

btw, do we need skipping it on windows?

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 think we tried removing this before and it failed 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

we should probably add a helper fixing the differing symbols on windows and unix so that the integration tests can run. separate issue, though

@SimenB
Copy link
Member

SimenB commented May 27, 2018

btw, wanna fix the lint warning while you're at it?

image

import bind from './bind';
import arrayEach from './array';
import templateEach from './template';

const each = (...args) => {
const each = (...args: any) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use mixed type 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.

I’m AFK at the moment, I’ll update this later tonight when I get home or in another PR I’ve got a few refactors/improvements I want to make with the each code 😄

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 tried to update to mixed and got the following flow error when trying to access args.length. I think it is fine to leave it as any

Cannot get `args.length` because property `length` is missing in mixed [1]. (index.js:15:12)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I think Array<mixed> should do the trick here, cause args is an array. Or we can be fancy and use $ReadOnlyArray<*>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice just updated :)

@SimenB
Copy link
Member

SimenB commented May 27, 2018

Failed circus on CI 😭

@SimenB
Copy link
Member

SimenB commented May 27, 2018

Green! Waiting on appveyor, but that doesn't run the each tests anyways

@codecov-io
Copy link

codecov-io commented May 27, 2018

Codecov Report

Merging #6309 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6309      +/-   ##
==========================================
+ Coverage   63.64%   63.67%   +0.02%     
==========================================
  Files         227      227              
  Lines        8638     8644       +6     
  Branches        3        4       +1     
==========================================
+ Hits         5498     5504       +6     
  Misses       3139     3139              
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-circus/src/index.js 66.66% <100%> (+5.12%) ⬆️
packages/jest-each/src/index.js 70.58% <100%> (ø) ⬆️

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 a43fd6c...4861776. Read the comment docs.

@@ -4,6 +4,7 @@

### Chore & Maintenance

* `[jest-circus]` Add dependency on jest-each ([#6309](https://github.com/facebook/jest/pull/#6309))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is now part of master, not 23.0.1 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed :)

@cpojer
Copy link
Member

cpojer commented May 28, 2018

please rebase.

@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.

6 participants