1249106 - Fail tests when React propType errors are logged

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[Lin Clark [:linclark]]

To ensure that APIs don't unintentionally drift, we should fail tests when React logs errors about propTypes.

:bgrins says that it's pretty easy to intercept the console.error messages.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

You can use something like the following:

https://dxr.mozilla.org/mozilla-central/source/devtools/shared/acorn/tests/unit/head_acorn.js#41-75

This will watch the messages.  You can trim down the listener to just do what you need.  This example does not actually fail a test, just dumps some things (some evildoer changed our console listeners to not fail anymore...).  Anyway, add a call to ok(false, ...) when you see a bad message.

Comment 2

[Lin Clark [:linclark]]

Attachment: Bug1249106.patch

Thanks for the pointer, that's exactly what I was looking for.

Unfortunately I can't seem to get it to fail on my local, even when I introduce a failed propType. My guess is that when I'm running the test, it's running with the production version of React, but I'm not sure.

Comment 3

[Lin Clark [:linclark]]

Attachment: failed-proptype.patch

Adding a patch that should in theory make it fail.

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Lin Clark [:linclark] from comment #2)
> Unfortunately I can't seem to get it to fail on my local, even when I
> introduce a failed propType. My guess is that when I'm running the test,
> it's running with the production version of React, but I'm not sure.

Do you have `ac_add_options --enable-debug-js-modules` set in your .mozconfig?

The example in https://dxr.mozilla.org/mozilla-central/source/devtools/shared/acorn/tests/unit/head_acorn.js#41-75 is a 'console service' listener which means it will listen for page errors - and I'm guessing that these warnings are going through the console API instead (console.warn, etc).  Here's a small test that sets up a console API listener: https://dxr.mozilla.org/mozilla-central/source/dom/tests/browser/browser_bug1004814.js

Comment 5

[Lin Clark [:linclark]]

Attachment: Bug1249106.patch

This now fails tests on debug builds if any of the props passed in don't pass propType validation. You can see an example here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=13765cf005e6&selectedJob=17214597

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8723435 [details] [diff] [review]
Bug1249106.patch

Review of attachment 8723435 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/framework/test/shared-head.js
@@ +50,5 @@
> +    var message = subject.wrappedJSObject.arguments[0];
> +
> +    // React has a few different type-related errors (which are output with
> +    // console.error). This regex should catch them all.
> +    if (/Failed.*Type/.test(message)) {

What other messages are possible?  I am little worried this regex is too general and could match on things we don't care about here.

The example try run only contained "Failed propType" in the messages, but maybe you're saying others are possible too?

@@ +61,5 @@
> +registerCleanupFunction(() => {
> +  Services.obs.removeObserver(ConsoleObserver, "console-api-log-event");
> +});
> +
> +

Nit: remove extra blank line

Comment 7

[Lin Clark [:linclark]]

You can see the other messages here: https://dxr.mozilla.org/mozilla-central/search?q=path%3Areact-dev+regexp%3AFailed.*Type&redirect=false&case=true

Comment 8

[Lin Clark [:linclark]]

Attachment: Bug1249106-fail-proptype.patch

Based on the feedback, I limited the check to failed proptypes. Eventually we might want to fail on more errors (and even more than I had originally included), but for now we can just fail on prop types.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8725491 [details] [diff] [review]
Bug1249106-fail-proptype.patch

Review of attachment 8725491 [details] [diff] [review]:
-----------------------------------------------------------------

Commit message needs bug number.

::: devtools/client/framework/test/shared-head.js
@@ +49,5 @@
> +  observe: function(subject, topic, data) {
> +    var message = subject.wrappedJSObject.arguments[0];
> +
> +    // React has a few different type-related errors (which are output with
> +    // console.error). This regex should catch them all.

Update comment, since we're only catching Failed propType now?

Comment 10

[Lin Clark [:linclark]]

Attachment: Bug1249106.patch

Thanks for catching that. Fixed.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/054bd8841f4c

Comment 12

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/a6b03f5e2c73 - the good news is, it works; the bad news is that I know it does because of

https://treeherder.mozilla.org/logviewer.html#?job_id=7784652&repo=fx-team
https://treeherder.mozilla.org/logviewer.html#?job_id=7783898&repo=fx-team
https://treeherder.mozilla.org/logviewer.html#?job_id=7783899&repo=fx-team
https://treeherder.mozilla.org/logviewer.html#?job_id=7784413&repo=fx-team

Comment 13

[Lin Clark [:linclark]]

yay... ?

I think this was probably introduced with a big commit to the memory tool on March 1. I'm working with them to fix it in Bug 1249106. Sorry about the confusion, and thanks for the helpful links.

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Filter on TEAPOT-SPLINES.

Comment 15

[Lin Clark [:linclark]]

This should be ok to check-in now.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=43e5a7bc022d

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/12637684d990

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/12637684d990