Disable WACG Errors if NODE_ENV === 'test'

[martinmckenna]

• I have searched the issues of this repository and believe that this is not a duplicate.

Summary 💡

I'd like the following errors to not be reported if the user's NODE_ENV === 'test':

Material-UI: the contrast ratio of 1.320754716981132:1 for #ffffff on #e0e0e0 falls below the WACG recommended absolute minimum contrast ratio of 3:1.
https://www.w3.org/TR/2008/REC-WCAG20-20081211/#visual-audio-contrast-contrast

Specifically wanting to update this line:

https://github.com/mui-org/material-ui/blob/4abf761591cf8b3cc13c51d3e9b624a5c3a2b475/packages/material-ui/src/styles/createPalette.js#L120

Motivation 🔦

I want to reduce console noise without having to do further configuration

[martinmckenna]

if it sounds like a reasonable idea, I can try and put a PR up for this

[eps1lon]

Could you elaborate why you want to silence this specific dev warning and why this is different to all the other dev warnings?

[martinmckenna]

I guess the main reason is that it's extremely noisy if you have a large app with many of these warnings, and while my team will most likely go through each of these errors and fix them, having a lot of these errors clutter your console when trying to run unit tests is pretty rough.

Ideally, I'd love a configuration option to silence them in test mode.

[martinmckenna]

Also unrelated: there's a typo in the error. It's WCAG, not WACG

[eps1lon]

I guess the main reason is that it's extremely noisy if you have a large app with many of these warnings, and while my team will most likely go through each of these errors and fix them, having a lot of these errors clutter your console when trying to run unit tests is pretty rough.

I understand that. My question was how this is different to all the other dev warnings. Shouldn't your test be as close to production as possible anyway? When is the color changed to meet the AA level?

[martinmckenna]

It's not different from any other console warning. I'm mainly just taking issue with how these warnings have really no relevance on most of (my) unit tests, since most of our tests aren't testing accessibility, but rather either an end-to-end flows or the existence/non-existence of UI elements.

I think not having this optional configurable is a significant annoyance to teams who'd like to tackle these warnings in piecemeal, rather than in one, giant PR.

Otherwise, we have one team member merge the material UI update with warnings, which clutters up everyone else's unit test console logs, until the warnings are resolved. If you're working on a large app, it pretty much enforces you to submit a huge PR to fix every warning site-wide in one shot.

[eps1lon]

I get all of that but this argument applies to any other warning. You're making an argument for no warnings in a test environment where you have the capability to silence these (e.g. stubbing console.error).

If you don't care about visuals in your unit tests then why use a custom theme in the tests in the first place? Why test with a custom theme for background color but not text color or vice versa?

since most of our tests aren't testing accessibility

I don't even have to encourage you to test this as well (since it is way easier than testing visuals). I personally would have difficulties making out a 1.3 contrast ratio and I do not have any visual impairment.

Again these are not just tiny warnings that should be put on a low priority backlog. Or warnings that need to be fixed in a lot of places. It's a single warning that can be fixed in a few places.

[oliviertassinari]

we have one team member merge the material UI update with warnings, which clutters up everyone else's unit test console logs, until the warnings are resolved.

@martinmckenna What version jump is causing this "spam" of warnings?

What about fixing the warning first then upgrading the Material-UI dependency? (2)

I would assume that if this approach (2) wasn't taken it's because it doesn't feel worth the trouble (meaning spending the time to look at what's wrong) instead, you would like to ignore it.
In this case, I believe the solution @eps1lon has proposed sounds great: stubbing console.error to ignore this specific message.

there's a typo in the error. It's WCAG, not WACG

Thank you :).

[martinmckenna]

@oliviertassinari I'm going from v4.4.2 to 4.6.0

Honestly the only reason I'm upgrading is because I'm also upgrade to TypeScript 3.7, which resulted in some type errors with MUI, so I ended up needing this commit:

a4862fb#diff-35ef3a60899e363b516b93bb7ba46b51

So really this is just a bad coincidence - upgrading MUI wasn't in my plan here.

Upgrading material-UI is sometimes a pain for teams like mine because we don't always have the time to address every and all warning right away, BUT we still want new features that are available in new versions of the package. Other priorities come up, and being able to configure where the warnings appear without having to stub console.warn or console.error would be amazing.

[oliviertassinari]

@martinmckenna It's strange, I'm not aware of any change that would explain these new warnings between these two versions. v4.4.2...v4.6.0#diff-9efe743b8cf918063d14bd692661844a
I suspect it hides something else.

Thank you for the feedback.

[oliviertassinari]

By the way @eps1lon, I believe we didn't agree on when console.error vs console.warn should be used: #15343 (comment).

[oliviertassinari]

Also, I remember @fzaninotto has a similar concern regarding the warning and error message of Material-UI.
@fzaninotto Are you happy with the way it works now? If not, what would you change? Thanks.

In any case, I think that the umbrella issue: #15343 could group this discussion.

[fzaninotto]

I didn't have the opportunity to see the latest changes; I'll report here if anything goes wrong when I do!