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

jasmine 2 #6769

Closed
wants to merge 4 commits into from
Closed

jasmine 2 #6769

wants to merge 4 commits into from

Conversation

aaronabramov
Copy link
Member

i took #5646 as a base

I removed MetaMatchers and instead ran jest on a specific file in a child process and compared the raw string output. It's not ideal, but much simpler than hacking into jasmine internals to compare the suites :)

cc: @zpao @cpojer

@cpojer
Copy link
Contributor

cpojer commented May 14, 2016

woah, this is awesome. I'll let the React folks merge this one, though :)

@aaronabramov aaronabramov changed the title Jest cli@12 jasmine 2 May 14, 2016
@ghost
Copy link

ghost commented May 14, 2016

@DmitriiAbramov updated the pull request.

@cpojer
Copy link
Contributor

cpojer commented May 14, 2016

@zpao @sebmarkbage can you think of a way to reduce the amount of permutation in ReactMultiChildText-test.js? It seems like it's taking 150 seconds because of the amount of work it is doing. Is all of this necessary?

@ghost
Copy link

ghost commented May 14, 2016

@DmitriiAbramov updated the pull request.

@sophiebits
Copy link
Collaborator

It is supposed to take less time. I posted a PR earlier today that fixes a severe perf issue. Still slow after the change but not that slow.

@cpojer
Copy link
Contributor

cpojer commented May 14, 2016

@spicyj where can I see your change? I can't find it.

@ghost
Copy link

ghost commented May 14, 2016

@DmitriiAbramov updated the pull request.

@sophiebits
Copy link
Collaborator

@cpojer Oops, you're right. I forgot to send it. Thanks. #6770

@aaronabramov aaronabramov force-pushed the jest-cli@12 branch 2 times, most recently from 0294471 to bf01b1c Compare May 14, 2016 07:47
@ghost
Copy link

ghost commented May 14, 2016

@DmitriiAbramov updated the pull request.

@gaearon
Copy link
Collaborator

gaearon commented May 15, 2016

@DmitriiAbramov Would you mind rebasing please?

@aaronabramov
Copy link
Member Author

aaronabramov commented May 15, 2016

@gaearon sure
yeah. it gets out of sync really quickly :)

@facebook-github-bot
Copy link

@DmitriiAbramov updated the pull request.

var aSpecs = (a.match(/it\s.*$/gm) || []).sort().join('\n');
var bSpecs = (b.match(/it\s.*$/gm) || []).sort().join('\n');
expect(aSpecs).toEqual(bSpecs);
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this is probably fine. Hacky but fine :). Should we make this fail if both specs are empty?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that this only checks the list of tests

  ✓ it preserves the name of the class for use in error messages
  ✓ it throws if no render function is defined
  ✓ it renders a simple stateless component with prop
  ✓ it renders based on state using initial values in this.props
  ✓ it renders based on state using props in the constructor
  ✓ it renders based on context in the constructor
  ✓ it renders only once when setting state in componentWillMount
  ✓ it should throw with non-object in the initial state property
  ✓ it should render with null in the initial state property
  ✓ it setState through an event handler
  ✓ it should not implicitly bind event handlers
  ✓ it renders using forceUpdate even when there is no state
  ✓ it will call all the normal life cycle methods
  ✓ it warns when classic properties are defined on the instance, but does not invoke them.
  ✓ it should warn when misspelling shouldComponentUpdate
  ✓ it should warn when misspelling componentWillReceiveProps
  ✓ it should throw AND warn when trying to access classic APIs
  ✓ it supports this.context passed via getChildContext
  ✓ it supports classic refs
  ✓ it supports drilling through to the DOM using findDOMNode

whereas the old one made sure the number of assertions matched. So this is significantly less strict than before.

Copy link
Member Author

Choose a reason for hiding this comment

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

@spicyj that is true.
Maybe what we can do is add an afterEach hook that will console.log the number of expects in the test and make output look something like:
✓ it supports classic refs [# of expects: 4]
and compare that.
hacky.. but maybe that'll do :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

That sounds good to me, can you do that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can merge after that's fixed.

@ghost
Copy link

ghost commented May 19, 2016

@DmitriiAbramov updated the pull request.

@aaronabramov
Copy link
Member Author

@spicyj i added a custom setup file that monkeypatches jasmine here https://github.com/facebook/react/pull/6769/files#diff-d14cc01bd4a76abb678ced92d16a0e9cR1

it depends on jestjs/jest#1027 and jestjs/jest#1028
which will hopefully be released tomorrow

This is how failure looks like
screen shot 2016-05-18 at 9 09 41 pm

it will also fail if any of the outputs are empty.

let me know if it makes sense and i'll rebase it again as soon as we ship the new version of jest

@ghost
Copy link

ghost commented May 20, 2016

@DmitriiAbramov updated the pull request.

@sophiebits
Copy link
Collaborator

I think that seems reasonable. Is there a way we can make the output show a more readable diff? Even if not, this seems good – thanks for doing the work!

@aaronabramov
Copy link
Member Author

@spicyj i'll work on the diff next, but that'll be a part of jest :)

@ghost
Copy link

ghost commented May 20, 2016

@DmitriiAbramov updated the pull request.

@aaronabramov
Copy link
Member Author

jest 12.1.0 is out
this PR is ready to be merged

@@ -51,7 +51,7 @@
"gulp-babel": "^6.0.0",
"gulp-flatten": "^0.2.0",
"gzip-js": "~0.3.2",
"jest-cli": "^12.0.2",
"jest-cli": "^12.1.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

can you make it jest?

@ghost
Copy link

ghost commented May 20, 2016

@DmitriiAbramov updated the pull request.

@gaearon
Copy link
Collaborator

gaearon commented May 25, 2016

Thanks a ton @DmitriiAbramov! I rebased this in #6872 and merged.

@gaearon gaearon closed this May 25, 2016
@cpojer
Copy link
Contributor

cpojer commented May 25, 2016

hell yeah! I think no one is using jasmine1 any more now, we'll make it optional in Jest and not download it by default any more :)

@cpojer
Copy link
Contributor

cpojer commented May 25, 2016

Also the tests should be 20 % faster or so :)

@gaearon
Copy link
Collaborator

gaearon commented May 25, 2016

Also the tests should be 20 % faster or so :)

Yes, it certainly seemed so to me. Great job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants