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 async/await to unit tests; modify server rendering tests to use async/await. #9089

Merged

Conversation

aickin
Copy link
Contributor

@aickin aickin commented Mar 1, 2017

Following on to #9055, this PR adds the ability to use async-await to unit tests, and it changes some of the existing server rendering tests to use async-await.

At @spicyj's suggestion in #9055, the transform is only applied to files in the __tests__ folders to avoid accidentally transforming shipping product code.

Copy link
Collaborator

@sophiebits sophiebits left a comment

Choose a reason for hiding this comment

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

Looks great assuming CI passes, just two inlines.

@@ -54,11 +55,17 @@ module.exports = {
!filePath.match(/\/node_modules\//) &&
!filePath.match(/\/third_party\//)
) {
// for test files, we also apply the async-await transform, but we want to
// make sure we don't accidentally apply that transform to product code.
var isTestFile = !!filePath.match(/__tests__/);
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 do .match(/\/__tests__\//)?

domElement.innerHTML = markup;
return domElement.firstChild;
});
errorCount);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: fb style is to have this closing ); on the next line, unindented

@aickin
Copy link
Contributor Author

aickin commented Mar 1, 2017

Thanks for the review; addressed in ec62549. Now crossing my fingers on CI.

@aickin
Copy link
Contributor Author

aickin commented Mar 3, 2017

Looks like CI has passed, and I think I've addressed all comments in the review. Can I humbly give this PR a nudge?

@sophiebits sophiebits merged commit fcf41e7 into facebook:master Mar 3, 2017
@sophiebits
Copy link
Collaborator

Yes of course! I don't get notified when CI runs :(

gaearon pushed a commit that referenced this pull request Mar 19, 2017
* Added a handful of SSR unit tests, ported from a previous pull request.

* Fixing linting errors

* Fixed a test helper function to properly report errors.

* Un-nested the new rendering tests. Updated the fiber test passing/not passing lists.

* Edited to comply with the react/jsx-space-before-closing eslint rule, which will soon be merged into master.

* Response to code review from @spicyj. Moved tests to separate file, reworked wording of test names, corrected use of canUseDom, and simplified tests against tagName. Thanks for the help, @spicyj!

* Converted the unit tests to use async-await style.

* Moved async-await babel transform for tests from .babelrc to preprocessor.js.

* Response to code review in PR #9089. Thanks, @spicyj!

* Fixing some bugs in the SSR unit tests.

* Missed deleting some repeated code in the last commit.

* Adding unit tests for property to attribute mapping in SSR.

* Removing some redundant unit tests.

* Oops. I forgot to re-run record-tests after c593dbc; fixing that here.

* Reformatting for prettier so that the build will pass.
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.

3 participants