[icons] Remove SvgIcon data-testid in production

[Janpot]

Removes the default data-testid prop when in production. Adds a section to the migration guide and remove the docs that describe this behavior.

[mui-bot]

Netlify deploy preview

• docs/data/material/components/icons/icons.md
• docs/data/material/migration/upgrade-to-v7/upgrade-to-v7.md

@material-ui/core: parsed: -0.07% 😍, gzip: -0.13% 😍

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against dd6fa25

[EfstathiadisD]

Terrible! Terrible folks! What on earth are y folks doing over there?

[Janpot]

What on earth are y folks doing over there?

@EfstathiadisD Trimming unnecessary bloat from production code, these test ids end up in server rendered html and potentially clash with user defined test ids.

The component still spreads props on the svg element, so a user defined testid will still end up there. Wouldn't that suffice for your use-case? Could you elaborate a bit more on why you think this should be kept?

[EfstathiadisD]

What on earth are y folks doing over there?

@EfstathiadisD Trimming unnecessary bloat from production code, these test ids end up in server rendered html and potentially clash with user defined test ids.

The component still spreads props on the svg element, so a user defined testid will still end up there. Wouldn't that suffice for your use-case? Could you elaborate a bit more on why you think this should be kept?

It was one of the most efficient ways to write tests!

We all had 1 way to test them, and now people are gonna start renaming it how they like.

I think options hurt the ecosystem more than strictness. I understand that it might not be beneficial but consider an approach where this Icons can have predefined data-testId.

Like a flag or something. And I suppose this goes to V7 as a breaking change. In our repo it about 35 instances of this.

And our apps aren't huge! Just IMHO!

[Janpot]

Like a flag or something.

Would it work for you if we add the test id only when process.env.NODE_ENV is not 'production'?

[mnajdova]

Add the test id only when process.env.NODE_ENV is not 'production'?

I think this is a good compromise, I can't imagine anyone wanting to have these in production.

[EfstathiadisD]

Like a flag or something.

Would it work for you if we add the test id only when process.env.NODE_ENV is not 'production'?

Yeah. That makes lots of sense! Thank You!!🙏

[hbj]

Hello! Removing it only for production is very confusing because if you run e2e tests with the production version as we do you might have your tests running locally when implementing them, because you use the dev version, but have your tests fail in CI that uses the prod version. If you forget about this point you might have to wait for the CI to finish to figure out the issue. This also makes debugging very difficult unless you're aware of this subtlety.

Is it possible to add some flag to opt out of removing the test-id?

[AndyAlkhawand]

Add the test id only when process.env.NODE_ENV is not 'production'?

I think this is a good compromise, I can't imagine anyone wanting to have these in production.

Add the test id only when process.env.NODE_ENV is not 'production'?

I think this is a good compromise, I can't imagine anyone wanting to have these in production.

Many companies use NODE_ENV = production for staging environments, such as QA, UAT etc.. This update can break cypress tests that validate MUI icons using the data-testid, that run on staging environments.