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 tests for ColorPalette component. #3887

Merged
merged 1 commit into from Dec 23, 2017

Conversation

Projects
None yet
2 participants
@BE-Webdesign
Contributor

BE-Webdesign commented Dec 9, 2017

Add tests for ColorPalette component.

const currentColor = 'red';
const onChange = jest.fn();
const wrapper = shallow( <ColorPalette colors={ colors } value={ currentColor } onChange={ onChange } /> );

This comment has been minimized.

@youknowriad

youknowriad Dec 11, 2017

Contributor

I can see why you're doing all these initializations outside the test calls but an issue with this way of writing the unit tests is that a test could alter these global variables and break the following tests. I'd suggest trying to keep each test as isolated as possible.

One way to do it to reinstantiate these variables in beforeEach

@youknowriad

youknowriad Dec 11, 2017

Contributor

I can see why you're doing all these initializations outside the test calls but an issue with this way of writing the unit tests is that a test could alter these global variables and break the following tests. I'd suggest trying to keep each test as isolated as possible.

One way to do it to reinstantiate these variables in beforeEach

This comment has been minimized.

@BE-Webdesign

BE-Webdesign Dec 11, 2017

Contributor

To clarify, change these to let and then rebind them in beforeEach() ?

@BE-Webdesign

BE-Webdesign Dec 11, 2017

Contributor

To clarify, change these to let and then rebind them in beforeEach() ?

This comment has been minimized.

@youknowriad

youknowriad Dec 12, 2017

Contributor

yep, I also don't mind repeating these small instantiations from test to test.

@youknowriad

youknowriad Dec 12, 2017

Contributor

yep, I also don't mind repeating these small instantiations from test to test.

This comment has been minimized.

@BE-Webdesign

BE-Webdesign Dec 14, 2017

Contributor

yep, I also don't mind repeating these small instantiations from test to test.

I am fine with that as well, but most of the tests in this project don't do that, and everyone else seems to be more in favor of this approach of declaring them at the top and reusing a single declaration, as it can help improve maintainability. Should we open an issue to figure out what we want to do to be consistent?

@BE-Webdesign

BE-Webdesign Dec 14, 2017

Contributor

yep, I also don't mind repeating these small instantiations from test to test.

I am fine with that as well, but most of the tests in this project don't do that, and everyone else seems to be more in favor of this approach of declaring them at the top and reusing a single declaration, as it can help improve maintainability. Should we open an issue to figure out what we want to do to be consistent?

This comment has been minimized.

@youknowriad

youknowriad Dec 18, 2017

Contributor

Not worth an issue IMO but feel free to do so if you think so. :) Just pick the option you like

@youknowriad

youknowriad Dec 18, 2017

Contributor

Not worth an issue IMO but feel free to do so if you think so. :) Just pick the option you like

This comment has been minimized.

@BE-Webdesign

BE-Webdesign Dec 18, 2017

Contributor

Just pick the option you like

I like consistency 😄

@BE-Webdesign

BE-Webdesign Dec 18, 2017

Contributor

Just pick the option you like

I like consistency 😄

@BE-Webdesign

This comment has been minimized.

Show comment
Hide comment
@BE-Webdesign

BE-Webdesign Dec 23, 2017

Contributor

Going to leave as is @youknowriad, as this is more in line with how all of the other tests are doing things. We definitely share the same thoughts regarding this, but to have better consistency, I think it might be best to leave this as is?

Let me know if that sounds good to you.

Contributor

BE-Webdesign commented Dec 23, 2017

Going to leave as is @youknowriad, as this is more in line with how all of the other tests are doing things. We definitely share the same thoughts regarding this, but to have better consistency, I think it might be best to leave this as is?

Let me know if that sounds good to you.

@youknowriad

Thanks for all these tests :)

@BE-Webdesign

This comment has been minimized.

Show comment
Hide comment
@BE-Webdesign

BE-Webdesign Dec 23, 2017

Contributor

Thanks for all these tests :)

Thank you for all of your work, Gutenberg is going to be great!

Contributor

BE-Webdesign commented Dec 23, 2017

Thanks for all these tests :)

Thank you for all of your work, Gutenberg is going to be great!

@BE-Webdesign BE-Webdesign merged commit 79a74be into master Dec 23, 2017

3 checks passed

codecov/project 39.27% (+0.14%) compared to 06317d7
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@BE-Webdesign BE-Webdesign deleted the add/test/blocks/color-palette branch Dec 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment