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 jest and react testing library tests for victory-line and victory-area #2196

Merged
merged 13 commits into from May 3, 2022

Conversation

becca-bailey
Copy link
Contributor

@becca-bailey becca-bailey commented Apr 14, 2022

This is some initial work to switch over tests from Karma + enzyme to Jest + RTL. This adds new tests, but doesn't delete the original tests just yet.

Completed packages:

  • victory-line
  • victory-area

Related issue: #2195

To run jest tests:

yarn nps jest

OR

yarn nps jest.native

To pick up code changes in Victory packages, this test command will need to be run concurrently with yarn nps watch.

@becca-bailey becca-bailey marked this pull request as draft April 14, 2022 23:00
@@ -31,7 +31,8 @@
"storybook": "nps storybook.server",
"build-storybook": "build-storybook",
"chromatic": "chromatic",
"chromatic-ci": "build-storybook && chromatic --ci --exit-zero-on-changes"
"chromatic-ci": "build-storybook && chromatic --ci --exit-zero-on-changes",
"test": "jest --config=jest-config.js"
Copy link
Member

Choose a reason for hiding this comment

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

So, there are two things really:

  1. Build all lib and test code when running tests.
  2. Build only test code when running tests. For lib code that means running a separate watch to rebuild or one-off build.

I think No. 2 is the way to go.

And for Jest TEST_MODULE doesn't really apply. And for Karma, it has very little actual speed impact. The thing that does now massively speed up Karma in my d3 branch is that I only use already build lib code and remove all transpiling and inclusion of source code.

@becca-bailey
Copy link
Contributor Author

@ryan-roemer Oops, I meant to edit my original question and accidentally deleted it! Anyway, I'm also looking into babel-jest which might give us some additional integration between our build step and jest watch mode. I agree that #2 makes more sense!

@ryan-roemer
Copy link
Member

@becca-bailey Cool! And if the jest tests run fast enough on their own, then we maybe don't even need to heavily optimize for jest watch. (And jest watch should be fine watching with <pkg>/es same as <pkg>/src for auto-rerunning changes).

@becca-bailey
Copy link
Contributor Author

@ryan-roemer That's a good point - for now running yarn nps watch concurrently with jest --watch works well!

@becca-bailey becca-bailey marked this pull request as ready for review April 29, 2022 18:41
@becca-bailey becca-bailey changed the title Add jest and react testing library tests (to replace karma + enzyme) Add jest and react testing library tests for victory-line and victory-area Apr 29, 2022
@becca-bailey
Copy link
Contributor Author

Any objections to merging in new tests as we go rather than waiting until they are all completed?

jest-config.js Outdated Show resolved Hide resolved
babel-config.js Outdated Show resolved Hide resolved
return value(props);
}
return value;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a minor change to the user-provided props that mimics the API of other Victory props and allows us to pass in a function that evaluates the props.

For example:

<VictoryLine aria-label={props => props.label} />

I added this small change to this PR because it was useful for testing, but I will make a note to add some additional documentation around this in a separate PR.

Comment on lines +176 to +181
const line = container.querySelector("path");
const renderedData = JSON.parse(line.getAttribute("data-json")).map(
({ t }) => t
);

expect(renderedData).toEqual([0, 1]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is kind of a hacky solution that allows us to test the rendered data, since we're no longer able to directly access props with Enzyme (for a good reason!).

I might do a bit of research to see if there are better approaches to testing rendered data in data visualization, since an automated test suite can't easily test things like is the line going up or down the same way we could test it visually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's also worth asking whether we should still be testing things like data ordering at the integration test level, or whether we should just unit test the helper functions.

@becca-bailey
Copy link
Contributor Author

I have some outstanding questions on how we want to approach some of these tests, but since this PR is making minimal code changes, I am okay merging this as-is as a jumping off point for future work. From there, we can re-evaluate which tests are most useful and make changes as needed.

@becca-bailey becca-bailey merged commit 593339e into main May 3, 2022
@becca-bailey becca-bailey deleted the feature/add-jest-and-rtl branch May 3, 2022 21:30
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.

None yet

2 participants