Skip to content
This repository was archived by the owner on Nov 14, 2020. It is now read-only.

[Fixes #19] Add Jest #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[Fixes #19] Add Jest #23

wants to merge 1 commit into from

Conversation

artgillespie
Copy link

RFC: Putting this up to get feedback on the approach before copying it to the extraction package.

@artgillespie artgillespie requested a review from hkasemir January 14, 2019 19:04
Copy link
Collaborator

@hkasemir hkasemir left a comment

Choose a reason for hiding this comment

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

Thanks for this! Looking great so far 😄

@@ -56,9 +55,8 @@
"eslint-plugin-jsx-a11y": "^6.0.2",
"eslint-plugin-react": "^7.4.0",
"fluent-react": "^0.8.1",
"istanbul": "^1.0.0-alpha",
"jest": "^23.6.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

so much cleaner, love it.

</Localized>
);
assert(wrapper.contains(expectedOutput));
expect(wrapper).toMatchSnapshot();
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍
I do kind of like the simple expectedOutput as a visual of what this component looks like. The snapshots themselves are hard to read. I think it's a reasonable tradeoff, though, for the thoroughness and simplicity of the test code.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I could go either way here, honestly.

import { pseudolocalize } from '../src';

describe('pseudolocalize', () => {
it('should transform a message', () => {
it.only('should transform a message', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

careful about leaving these in.

Is there a linting rule or similar to prevent accidentally merging this in to master?

Copy link
Author

Choose a reason for hiding this comment

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

D'oh! :shame:

There is a linting rule. I will find it and add.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants