Skip to content

Native Flow, use Jest #767

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.

Sign up for GitHub

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

Jump to bottom
Merged
tmcw merged 2 commits into from
May 9, 2017
Merged

Native Flow, use Jest #767

tmcw merged 2 commits into from
May 9, 2017

Conversation

@tmcw
Copy link
Member

@tmcw tmcw commented May 1, 2017 •
edited
Loading

@tmcw tmcw changed the title Native Flow comments Native Flow, use Jest May 1, 2017
@tmcw tmcw requested review from arv and jfirebaugh May 2, 2017 15:54
@tmcw
Copy link
Member Author

tmcw commented May 2, 2017

Would much appreciate a code review if y'all can spare the time! This is a big change, but has mainly accomplished:

  • Using native Flow annotations
  • Switching from node-tap to Jest and using Jest's snapshot tests, and keeping code coverage in place - though it now only tests directly unit-tested code, rather than counting integration tests in the same bucket
  • Sets up babel to transpile the library. It's restrictive - not going crazy on JS features. I'd love to keep it strict enough that we could eventually use Rollup to distribute a bundle.
  • Tests are faster now: 2:48 instead of 3:20

arv
arv approved these changes May 6, 2017
View reviewed changes
Copy link
Contributor

@arv arv left a comment

Choose a reason for hiding this comment

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

Overall an improvement. I had a few minor questions but nothing that is preventing you from going forward with this.

__tests__/bin-readme.js Outdated
});
}

var UPDATE = !!process.env.UPDATE;
Copy link
Contributor

@arv arv May 6, 2017

Choose a reason for hiding this comment

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

Do we still need this?

We (I) can change this in a follow up if it works now. I don't want to make this PR any larger.

Copy link
Member Author

@tmcw tmcw May 8, 2017

Choose a reason for hiding this comment

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

👍 removed

__tests__/lib/merge_config.js Outdated
mergeConfig = require('../../src/merge_config');

test('bad config', function(t) {
test('bad config', function(done) {
Copy link
Contributor

@arv arv May 6, 2017

Choose a reason for hiding this comment

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

FYI, Jest also supports using async functions.

package.json Outdated
"self-lint": "node ./bin/documentation.js lint",
"test": "eslint lib && are-we-flow-yet lib && flow check && npm run self-lint && npm run test-tap",
"test-tap": "tap -t 120 --coverage --nyc-arg=--cache test/*.js test/lib test/streams"
"test": "eslint src && are-we-flow-yet src && flow check && npm run self-lint && jest --runInBand"
Copy link
Contributor

@arv arv May 6, 2017

Choose a reason for hiding this comment

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

Do we really need --runInBand? That slows things down a bit

Copy link
Member Author

@tmcw tmcw May 8, 2017

Choose a reason for hiding this comment

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

--runInBand makes tests way more reliable and faster on CircleCI, but slower locally. I'd love to find a way to only enable it in the CircleCI environment, but haven't found one yet.

src/serve/server.js Outdated
pify = require('pify'),
EventEmitter = require('events').EventEmitter,
liveReload = require('tiny-lr'),
// liveReload = require('tiny-lr'),
Copy link
Contributor

@arv arv May 6, 2017

Choose a reason for hiding this comment

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

?

src/serve/server.js Outdated

return Promise.all([
pify(this._lr.listen.bind(this._lr))(35729),
// this._lr && pify(this._lr.listen.bind(this._lr))(35729),
Copy link
Contributor

@arv arv May 6, 2017

Choose a reason for hiding this comment

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

?

src/serve/server.js Outdated
* @param port server port to serve on.
*/
class Server extends EventEmitter {
/* :: _lr: Object; */
Copy link
Contributor

@arv arv May 6, 2017

Choose a reason for hiding this comment

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

Can this use non comment syntax now?

.eslintrc Outdated
"no-self-compare": 2,
"no-void": 2,
"no-unused-vars": 2,
"no-unused-vars": 1,
Copy link
Contributor

@arv arv May 6, 2017

Choose a reason for hiding this comment

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

😿

Copy link
Member Author

@tmcw tmcw May 8, 2017

Choose a reason for hiding this comment

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

eslint was, for some reason, incorrectly flagging variables as unused, but trying it today.. it works! so back to 2.

__tests__/bin.js
@@ -1,4 +1,4 @@
'use strict';
/* global jasmine */
Copy link
Contributor

@arv arv May 6, 2017

Choose a reason for hiding this comment

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

?

declarations/comment.js Outdated
file: string
};

// type InputsConfig = {
Copy link
Contributor

@arv arv May 6, 2017

Choose a reason for hiding this comment

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

?

Copy link
Member Author

@tmcw tmcw May 9, 2017

Choose a reason for hiding this comment

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

👍 Removed

@coveralls
Copy link

coveralls commented May 8, 2017

Coverage Status

Changes Unknown when pulling 6ef60e0 on native into ** on master**.

@tmcw
build: Use Flow syntax without comments.
6adabd5 
We're switching to Flow annotations - not Flow comments. This
gives documentation.js the ability to self-document without
JSDoc types and improves our compatibility with tools like
prettier.

Fixes #729. Fixes #709
@tmcw
Copy link
Member Author

tmcw commented May 9, 2017

Thanks for the review, Erik!

@tmcw tmcw merged commit 11d9045 into master May 9, 2017
@coveralls
Copy link

coveralls commented May 9, 2017

Coverage Status

Changes Unknown when pulling ad8dc3e on native into ** on master**.

@coveralls
Copy link

coveralls commented May 9, 2017

Coverage Status

Changes Unknown when pulling 3a91eec on native into ** on master**.

@coveralls
Copy link

coveralls commented May 9, 2017

Coverage Status

Changes Unknown when pulling 6adabd5 on native into ** on master**.

@coveralls
Copy link

coveralls commented May 9, 2017

Coverage Status

Changes Unknown when pulling 9b1c41b on native into ** on master**.

@tmcw tmcw mentioned this pull request Jun 8, 2017
Closed
@tmcw tmcw deleted the native branch April 11, 2018 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@arv arv arv approved these changes

@jfirebaugh jfirebaugh Awaiting requested review from jfirebaugh

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tmcw @coveralls @arv