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

[test] Add toWarnDev() and toErrorDev() matcher #21581

Merged
merged 15 commits into from
Jul 2, 2020

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Jun 26, 2020

Currently we simply throw in console.error and console.warn calls. This creates hard to parse stacktraces where the origin of the console call is deeply hidden in the stacktrace. We also have a dedicated helper to fully mock and silence console.error calls e.g. https://app.circleci.com/pipelines/github/mui-org/material-ui/14984/workflows/928a1113-8c3b-45a9-8655-db59d54bc14d/jobs/160938/steps. This enables problematic tests suites where only a subset of the messages is asserted or errors are completely ignored.

We now provide better stacktraces for unexpected error calls:
unexpected console.error call

Documentation: test/console-error/test/README.md#unexpected-calls-to-consoleerror-or-consolewar

This PR also introduces dedicated toWarnDev and toErrorDev helpers which require that every message is expected. You could still trick it with empty messages but this makes the problem stand out a bit more.

Documentation for toErrorDev: test/console-error/test/README.md#writing-a-test-for-consoleerror-or-consolewarn
Test suite for toErrorDev (showcasing stacktraces): test/console-error/test/utils/initMatchers.test.js

/cc @mui-org/core to check out https://github.com/eps1lon/material-ui/blob/test/console-error/test/README.md#unexpected-calls-to-consoleerror-or-consolewar and https://github.com/eps1lon/material-ui/blob/test/console-error/test/README.md#writing-a-test-for-consoleerror-or-consolewarn and comment if there are any questions.

@eps1lon eps1lon added the test label Jun 26, 2020
@eps1lon eps1lon marked this pull request as ready for review June 26, 2020 12:09
@eps1lon eps1lon requested review from oliviertassinari, mnajdova and joshwooding and removed request for oliviertassinari and mnajdova June 26, 2020 12:10
// eslint-disable-next-line no-underscore-dangle
const callback = this._obj;

if (process.env.NODE_ENV !== 'production') {
Copy link
Member

Choose a reason for hiding this comment

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

What's the motivation for a different assertion logic if node env is production?

Copy link
Member Author

Choose a reason for hiding this comment

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

In prod it shouldn't match any console calls because they are stripped out. At least so far we don't have any console calls that are kept in prod bundles. Though we don't run tests on the prod bundle so it's not doing anything. Just implementing the semantics of a "to error in development" matcher.

Copy link
Member

@joshwooding joshwooding left a comment

Choose a reason for hiding this comment

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

Love the new API. One small thing though

test/README.md Outdated Show resolved Hide resolved
@mui-pr-bot
Copy link

mui-pr-bot commented Jun 27, 2020

Details of bundle changes

Generated by 🚫 dangerJS against 8631084

@oliviertassinari oliviertassinari merged commit 45cb77f into mui:next Jul 2, 2020
@eps1lon eps1lon deleted the test/console-error branch July 6, 2020 08:10
]);
}
// eslint-disable-next-line no-console
console[methodName] = logUnexpectedConsoleCalls;
Copy link
Member

@oliviertassinari oliviertassinari Jul 12, 2020

Choose a reason for hiding this comment

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

Something is off with the overall logic. If you run the tests in watch mode and log something in the console, the tests don't fail. However, if you do the same outside the watch mode, it fails correctly.

We can reproduce the issue with this diff and yarn test:watch, yarn test:unit.

diff --git a/packages/material-ui-lab/src/Alert/Alert.test.js b/packages/material-ui-lab/src/Alert/Alert.test.js
index c4de7927fd..9a88e421c8 100644
--- a/packages/material-ui-lab/src/Alert/Alert.test.js
+++ b/packages/material-ui-lab/src/Alert/Alert.test.js
@@ -5,7 +5,7 @@ import describeConformance from '@material-ui/core/test-utils/describeConformanc
 import Alert from './Alert';
 import Paper from '@material-ui/core/Paper';

-describe('<Alert />', () => {
+describe.only('<Alert />', () => {
   const mount = createMount();
   let classes;

@@ -20,4 +20,8 @@ describe('<Alert />', () => {
     refInstanceof: window.HTMLDivElement,
     skip: ['componentProp'],
   }));
+
+  it('should work', () => {
+    console.error('oops');
+  });
 });

Copy link
Member Author

Choose a reason for hiding this comment

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

This is an issue with mocha: mochajs/mocha#4347

Copy link
Member

Choose a reason for hiding this comment

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

Upvoted, thanks for the reference.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not going to work on that issue since it's trivial to get working: explicitly expect an error or negate that expectation. A test without assertion is not a placeholder for "no error".

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think that we can wait for mocha to solve it :). It took me some time to understand what was going wrong between the CI in my local env. Now, it's documented

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Should've included yarn test:watch but I never use it. Should've explicitly requested review of this file so that we're all on the same page.

Copy link
Member

Choose a reason for hiding this comment

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

👌

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

Successfully merging this pull request may close these issues.

4 participants