Skip to content

Use jest for unittesting - standardize unit testing epic#904

Merged
shilman merged 60 commits intomasterfrom
jest-updated
Apr 17, 2017
Merged

Use jest for unittesting - standardize unit testing epic#904
shilman merged 60 commits intomasterfrom
jest-updated

Conversation

@ndelangen
Copy link
Copy Markdown
Member

@ndelangen ndelangen commented Apr 16, 2017

Issue:

What I did

  • UPDATED a lot of packages
  • ADDED jest tested to the root
  • REMOVED unit-testing dependencies from packages
  • CHANGED tests to run in jest
  • MOVED examples out of packages

aaronmcadam and others added 30 commits April 9, 2017 18:39
So far, we've set up the tools and have one failing test inside
storyshots.
The mocks here a bit complicated, so I'm leaving them for later
# Conflicts:
#	.eslintrc
#	.gitignore
#	package.json
#	packages/addon-knobs/package.json
#	packages/addon-knobs/yarn.lock
#	packages/react-storybook/package.json
#	packages/storybook-ui/package.json
#	packages/storybook-ui/src/modules/api/actions/__tests__/api.js
#	packages/storybook-ui/src/modules/shortcuts/actions/__tests__/shortcuts.js
#	packages/storybook-ui/src/modules/ui/actions/__tests__/ui.js
#	packages/storybook-ui/src/modules/ui/configs/__tests__/handle_keyevents.js
#	packages/storybook-ui/src/modules/ui/configs/__tests__/handle_routing.js
#	packages/storybook-ui/src/modules/ui/configs/__tests__/init_panels.js
#	packages/storyshots/package.json
#	packages/storyshots/stories/__test__/__snapshots__/storyshots.test.js.snap
@ndelangen ndelangen force-pushed the jest-updated branch 3 times, most recently from 6f6a652 to 13b0938 Compare April 16, 2017 21:08
Copy link
Copy Markdown
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Approved but with some caviats. See comments.

Comment thread .travis.yml Outdated
after_success: ./scripts/travis/after_success.sh
script: npm run lint && npm run test
script:
- "npm run lint"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

import { expect } from 'chai';
import ClientAPI from '../client_api';

const { describe, it } = global;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ndelangen don't we want this across the board?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What do you mean here @shilman?

@@ -1,6 +1,4 @@
import actions from '../shortcuts';
import { expect } from 'chai';
const { describe, it } = global;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ditto ;-)

describe('manager.ui.containers.layout', () => {
describe('mapper', () => {
it('should give correct data', () => {
test('should give correct data', () => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why test? should be it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

it should

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

test is the default and mentioned all over the documentation.

@@ -1,2 +0,0 @@
import initStoryshots from '../../src';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

are we not testing storyshots? is this temporary?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The way they were tested was just a smoke test, which breaks now because the main index isn't executed within the context of that package's package.json

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yes temporary indeed, the implementation was a bad smoke-test. Someone needs to do a thorough analysis on how to test it.

Copy link
Copy Markdown
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

If I change .eslintrc.js to extend airbnb-base, it shows me lots of linting errors.

  extends: [
    'airbnb-base',
  ],

I believe the current configuration is incorrectly swallowing a lot of errors.

@aaronmcadam
Copy link
Copy Markdown
Contributor

@ndelangen I see you're trying out yerna, what was the motivation there? The author himself says it's quite a hacky solution.

@ndelangen
Copy link
Copy Markdown
Member Author

Ok, @shilman I agree, linting can be made much more strict then it is now. I will setup a new branch and PR for that. I think we can merge this and then start working on improving linting?

@ndelangen ndelangen mentioned this pull request Apr 17, 2017
14 tasks
@shilman
Copy link
Copy Markdown
Member

shilman commented Apr 17, 2017

OK @ndelangen I agree with that solution, because we are doing heavy lifting on the monorepo transition and it's better to have these big changes merged. But FYI, I added a line in some source file that said only foo and the linter didn't throw any error. So I think it's a high priority change. I'm glad to see you're already working on it.

@nx-cloud
Copy link
Copy Markdown

nx-cloud bot commented Apr 20, 2025

View your CI Pipeline Execution ↗ for commit bfd9d26.

Command Status Duration Result
nx run-many -t build --parallel=3 ✅ Succeeded 1m 37s View ↗

☁️ Nx Cloud last updated this comment at 2025-04-20 13:46:36 UTC

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