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

chore(project): add setup for test failure with unexpected console #5184

Conversation

joshblack
Copy link
Contributor

This PR adds some config to our Jest environment to fail tests that emit console.{error,warn} while executing. In CI, this setup will also fail tests that emit console.log. This is useful for cleaning up our test output, in addition to pointing to inconsistencies in our tests where props may be incorrectly applied.

In order to get the tests to go green, I also needed to update tests that failed these new checks. These tests more often than not were missing props, or adding props to DOM nodes that were meant for specific child components.

Changelog

New

  • Add setup in Jest that fails on consle.{error,warn,log} statements (log is only in CI so that developers can debug locally)

Changed

  • Update tests that failed console.* checks

Removed

@joshblack joshblack requested a review from a team as a code owner January 26, 2020 01:36
@ghost ghost requested review from asudoh and emyarod January 26, 2020 01:36
@netlify
Copy link

netlify bot commented Jan 26, 2020

Deploy preview for the-carbon-components ready!

Built with commit 29cac6b

https://deploy-preview-5184--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Jan 26, 2020

Deploy preview for carbon-elements ready!

Built with commit 29cac6b

https://deploy-preview-5184--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Jan 26, 2020

Deploy preview for carbon-components-react ready!

Built with commit 29cac6b

https://deploy-preview-5184--carbon-components-react.netlify.com

@joshblack
Copy link
Contributor Author

Dependant on: #5183

@@ -22,7 +22,7 @@ const files = glob.sync('**/*.scss', {
const render = promisify(sass.render);

describe('styles', () => {
jest.setTimeout(8000);
jest.setTimeout(10000);
Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW I had to change this to 12000 to not get an error 🤷‍♂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooof, same. Let me bump up to 20000 just in case

Copy link
Collaborator

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

LGTM! 👍 ✅

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

LGTM basically, just wondered if utilizing PR# 4045 in https://github.com/facebook/jest is desirable.

@joshblack
Copy link
Contributor Author

@asudoh yeah! spyOn is perfect, the mock just needs to be restored after being used. I think DatePicker is a good example: https://github.com/carbon-design-system/carbon/pull/5184/files#diff-209ead5f5d1e9733be537c4a0684ef04R333

@asudoh
Copy link
Contributor

asudoh commented Jan 27, 2020

Just in case it wasn't clear enough - I meant auto-restoring mocks (e.g. upon global after-each).

@joshblack joshblack merged commit b3a2c84 into carbon-design-system:master Jan 27, 2020
@joshblack joshblack deleted the chore/add-warning-for-console-in-test branch January 27, 2020 22:53
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