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

Jest Test Suite Set-up and some tests #109

Merged
merged 23 commits into from
Aug 24, 2018
Merged

Jest Test Suite Set-up and some tests #109

merged 23 commits into from
Aug 24, 2018

Conversation

amycheng
Copy link
Contributor

@amycheng amycheng commented Jul 13, 2018

related #20

This PR includes:

  • setting up the Jest test suite
  • upgrading CircleCI config to v. 2
  • tests for some services, specifically for the add, create, remove, toggle services
    • BONUS * Added treeshaking for lodash

The kind of feedback I'm looking for:

  • do the config files for Jest and CircleCI look correct? Could they be written better?
  • are we testing the correct things? Do the test names make sense? I am hoping that the tests serve as decent documentation as to what these services are doing.
  • is there a better way to mock dependencies?

Notes:
You'll notice that a lot of tests are asserting if a dispatch object is being defined correctly. clay-space-edit is a plugin that hooks into Kiln's Vuex (the thing we're using do state management in Kiln) store and in order to trigger things that Kiln handles (like updating components), clay-space-edit sends a dispatch to trigger actions. Instead of mocking all of Vuex, I'm just sending dispatches to an array, here

]
],
"env": {
"test": {
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, it looks like there's no env stuff being set in the package.json. Is this being called correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did some digging and there's no need to set a babel env in package.json because Jest will automatically set the env to test if it's not provided (per the doc here).

command: npm run lint
- run:
name: Run tests
command: npm run test
Copy link
Contributor

Choose a reason for hiding this comment

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

we should probably use parallelism: 4 and --maxWorkers=4 for jest to make use of it's parallel functionality. There's a good example in the kiln config.

Also, are we checking code coverage (via coveralls/etc) when we run tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, will add

"overrides": [
{
"files": ["src/**/*.js", "src/**/*.vue"],
"excludedFiles": "*.test.js"
Copy link
Contributor

Choose a reason for hiding this comment

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

why are tests excluded from eslint?

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 linting for TODO statements, but would like to leave them in tests as a breadcrumb for me or the next developer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I'm worried that this might lead to issues down the line, but 👍 for now

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'll try to remember to lint test files again when we have decent test coverage

@@ -1,6 +1,7 @@
import { getAvailableComponents, findParentUriAndList } from './utils';
import { findSpaceLogic } from './create-service';
import { setNewActive } from '../services/toggle-service';
import { setNewActiveLogic } from '../services/toggle-service';
import { last } from 'lodash';
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this will actually tree-shake lodash when all of clay-space-edit is compiled, will it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh I think you're right, webpack is not doing any tree-shaking. Will investigate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

K, added Lodash tree-shaking

@@ -99,7 +101,8 @@ function componentToSpace(store, ref, parentRef, spaceName) {
]
});
})
.catch(function () {
.catch(function (res) {
console.log(res);
Copy link
Contributor

Choose a reason for hiding this comment

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

extra console log?

if (activeUri) {
dom.find(`[data-uri="${activeUri}"]`).setAttribute(activeAttr, '');
}
console.log('foo');
Copy link
Contributor

Choose a reason for hiding this comment

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

extra console.log?

dispatchCalls = [];
});

// TODO: these tests are WIP
Copy link
Contributor

Choose a reason for hiding this comment

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

are these going to be a part of this PR, or a separate one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

going to be a separate one

nelsonpecora
nelsonpecora previously approved these changes Jul 17, 2018
@amycheng
Copy link
Contributor Author

related to #20

@nelsonpecora
Copy link
Contributor

We do cherry-picking in Kiln, but I think that requires doing import _ from 'lodash' rather than using destructured imports. Also, the defaults for the webpack plugin are pretty strict, so we manually set them here: https://github.com/clay/clay-kiln/blob/master/webpack.config.js#L15

@amycheng
Copy link
Contributor Author

lolo oops forgot to merge this in

@amycheng amycheng merged commit b9373b0 into master Aug 24, 2018
@amycheng amycheng deleted the testing branch August 24, 2018 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants