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

Tests for fixed bug in unsubscribing consumers #8

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

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Feb 1, 2018

cc @TrySound ;)

@@ -41,7 +41,7 @@
"enzyme-to-json": "^3.2.2",
"flow-bin": "^0.60.1",
"husky": "^0.14.3",
"jest": "^21.2.1",
"jest": "^22.1.4",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for a moment I thought there was no test setup in this package (confused it with unstated repo), so I've started creating one and I've bumped jest version by doing so - can revert that change if you dont like it

Copy link

@nzec nzec left a comment

Choose a reason for hiding this comment

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

Looks fine!

expect(wrapper).toMatchSnapshot();
wrapper.find('button').simulate('click');
expect(wrapper).toMatchSnapshot();
})
Copy link
Owner

Choose a reason for hiding this comment

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

It's not really clear that this tests unsubscribing the right consumer. Instead of modifying the existing component, could you create a new one that wraps two different <Title>'s and hides one of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what is the idea behind proposed tweaked test - I mean I'm not sure in what way would you see it tweaked.

This test is testing that correct component got unsubscribed (or rather that others are still subscribed). Each <Title/> has different children, so we can see if those components got updated. I agree though that this might not be as clear as it could be - mainly because it's a snapshot test.

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.

4 participants