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

Replacing karma+mocha with jest+enzyme. #4162

Merged
merged 4 commits into from Sep 29, 2017

Conversation

Projects
None yet
3 participants
@dennisoelkers
Member

dennisoelkers commented Sep 19, 2017

Description

Motivation and Context

This change is:

  • Replacing previously little used karma + mocha with jest + enzyme, which allows us to do snapshot testing
  • Updating enzyme
  • Converting existing tests to work with jest
  • Making maven run jest for tests
  • Establishing consistent directory structure for tests (tests for src/foo/bar/Baz.jsx are in test/foo/bar/Baz.test.js)

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
Replacing karma+mocha with jest+enzyme.
This change is:

  * Replacing previously little used karma + mocha with jest + enzyme,
which allows us to do snapshot testing
  * Updating enzyme
  * Converting existing tests to work with jest
  * Making maven run jest for tests
  * Establishing consistent directory structure for tests (tests for
`src/foo/bar/Baz.jsx` are in `test/foo/bar/Baz.test.js`)

dennisoelkers added some commits Sep 20, 2017

@edmundoa edmundoa self-assigned this Sep 22, 2017

@edmundoa

This comment has been minimized.

Member

edmundoa commented Sep 27, 2017

This looks good to me and I think it can come in handy in some cases, good work!

The only thing I would change is the placement of tests. I think having another top level folder with the whole tree structure for tests is a bit annoying, specially in JS, where the IDE only helps you so much. Editing a test would require jumping on the tree, and moving a file would require moving the test as well, recreating folders and so on as you go. I don't know about others, but I will most likely forget about it and the tests tree will probably end up in an inconsistent state.

Did you consider putting the test file along the code is testing? We do that for styles now and I think it works quite well. Here I represent both ways with a couple of trees, in case I didn't make my point clear.

Current folder structure:

├── src
│   ├── components
│   │   ├── common
│   │   │   ├── Spinner.jsx
│   │   ├── users
│   │   │   ├── UserPreferencesButton.jsx
│   ├── logic
│   │   └── search
│   │       ├── queryParser.ts
├── test
│   ├── components
│   │   ├── common
│   │   │   ├── Spinner.test.jsx
│   │   │   └── __snapshots__
│   │   │       └── Spinner.test.jsx.snap
│   │   └── users
│   │       ├── UserPreferencesButton.test.js
│   │       └── __snapshots__
│   │           └── UserPreferencesButton.test.js.snap
│   └── logic
│       └── search
│           └── queryParser.test.js

Having both tests and source in the same level:

├── src
│   ├── components
│   │   ├── common
│   │   │   ├── Spinner.jsx
│   │   │   ├── Spinner.test.jsx
│   │   │   └── __snapshots__
│   │   │       └── Spinner.test.jsx.snap
│   │   ├── users
│   │   │   ├── UserPreferencesButton.jsx
│   │       ├── UserPreferencesButton.test.js
│   │       └── __snapshots__
│   │           └── UserPreferencesButton.test.js.snap
│   ├── logic
│   │   └── search
│   │       ├── queryParser.ts
│           └── queryParser.test.js

Any opinions on this?

@bernd

This comment has been minimized.

Member

bernd commented Sep 27, 2017

I like the second example with the colocation of source code and tests. I already get lost with our current system where we group web source code by type and not by sub-system. Having another code tree would be even more confusing, indeed.

@dennisoelkers

This comment has been minimized.

Member

dennisoelkers commented Sep 27, 2017

Great to hear that you prefer putting it next to the SUT, because this is actually a best practice in JS/jest-world. I hesitated from doing this, because I actually thought people would expect a directory layout that is consistent with our Java code base (separate tests directory).

👍 for tests next to the SUT! I would be happy to do the change for this.

@edmundoa

This comment has been minimized.

Member

edmundoa commented Sep 27, 2017

Good to hear that! 👍

This is also why I started the conversation, in the end it's a matter of taste more than anything else. @dennisoelkers let me know if you need some help with it.

@dennisoelkers

This comment has been minimized.

Member

dennisoelkers commented Sep 29, 2017

Implemented the change.

@edmundoa

LGTM 👍

@edmundoa edmundoa merged commit 14218bc into master Sep 29, 2017

4 of 5 checks passed

graylog-project/pr Jenkins build graylog-project-pr-snapshot 522 has failed
Details
ci-web-linter Jenkins build graylog-pr-linter-check 1953 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@edmundoa edmundoa deleted the add-jest-for-js-testing branch Sep 29, 2017

edmundoa added a commit that referenced this pull request Sep 29, 2017

Replacing karma+mocha with jest+enzyme. (#4162)
* Replacing karma+mocha with jest+enzyme.

This change is:

  * Replacing previously little used karma + mocha with jest + enzyme,
which allows us to do snapshot testing
  * Updating enzyme
  * Converting existing tests to work with jest
  * Making maven run jest for tests
  * Establishing consistent directory structure for tests (tests for
`src/foo/bar/Baz.jsx` are in `test/foo/bar/Baz.test.js`)

* Adding mocking helpers.

* Adding default rules parameter value, correcting condition.

* Putting tests next to components under test.

(cherry picked from commit 14218bc)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment