[styles] Fix resolution of default props

[oliviertassinari]

I have found this bug trying to customize the small and large variant of the button in #24095 for this spec:

See this reproduction for the issue: https://codesandbox.io/s/globalthemevariants-material-demo-forked-rexjl?file=/demo.tsx

import * as React from "react";
import { createMuiTheme, ThemeProvider } from "@material-ui/core/styles";
import Button from "@material-ui/core/Button";

const theme = createMuiTheme({
  components: {
    MuiButton: {
      styleOverrides: {
        sizeSmall: {
          backgroundColor: "red"
        }
      }
    }
  }
});

export default function GlobalThemeVariants() {
  return (
    <ThemeProvider theme={theme}>
      <Button variant="contained" size="small">
        Small
      </Button>
    </ThemeProvider>
  );
}

Once we apply the patch of this pull request: https://codesandbox.io/s/globalthemevariants-material-demo-forked-13dfi?file=/package.json

Regarding the tests, I'm not sure we need any, it seems to be simply about using the same approach as we use for writing the CSS but for the overrides.

Closes #24262

[mui-pr-bot]

Details of bundle changes

Generated by 🚫 dangerJS against 03aca94

[eps1lon]

This should be fixed with a test.

[oliviertassinari]

Would it work if we add the test case for only one component, as a meant to validate that the whole integration is working? We could rely on the fact that the other components should all implement the same template.

This was the approach used since v1 for withStyles, the Button component was serving this purpose (as far as I remember). The solution seems simple enough to make me think that adding a new entry into describe conformance won't be efficient (spending more time writing test cases than saving us time and trust with regressions).

[eps1lon]

Would it work if we add the test case for only one component, as a meant to validate that the whole integration is working?

A test should be added for the behavior that was broken.

[mnajdova]

Changes looks good. The only reason the styleProps were not used, was because they were introduced later. I’d say we should add entry in the conformance suite that test that default props work for the overrides (that way even if we change the implementation it would safe us from making bug like this in the future).

test case added

[eps1lon]

Badge is still untested.

[eps1lon]

When should this not be tested?

[oliviertassinari]

If you ask me, I would say most of the time we don't need to test it. We don't even fully test it right now, we take an arbitrary case. I would personally prefer we use the Button or Slider as a stress test template. It should be enough.

Badge is still untested.

The badge doesn't have state on its root element.

[oliviertassinari]

I have moved away from the API in #24095 with this patch https://gist.github.com/oliviertassinari/60614c8c37d4ba10adc2a3b1aba1277e. The fix is meant for backward compatibility with v4.

[eps1lon]

Badge is still untested.

The badge doesn't have state on its root element.

Then why did we need to change the implementation?

If you ask me, I would say most of the time we don't need to test it.

It already broke once.

The fix is meant for backward compatibility with v4.

describeConformanceV5 is not meant for v4.

[oliviertassinari]

It already broke once.

Exactly why I suggest the current tradeoff. If it breaks a second time, it will be a signal that it isn't enough.

[eps1lon]

The argument I have tried to defend is that it doesn't matter.

It does matter. Every implementation needs a test unless it isn't testable at which point one needs to make an argument why it isn't testable.

We don't decide by who writes the code if it needs to be tested.

[oliviertassinari]

Every implementation needs a test unless it isn't testable

I respectfully disagree. I think that we should also add "unless it's not worth it".

I think that it's like the 100% code coverage I have tried to push in the past and we gave up on. It's not always worth the time, once the opportunity cost is high, best to defer and be lazy.

[eps1lon]

I respectfully disagree. I think that we should also add "unless it's not worth it".

Impossible to collaborate on that premise since this argument applies anytime. Also: please stop cutting the quote. This is not what I said.

I think that it's like the 100% code coverage I have tried to push in the past and we gave up on.

At some point there comes a time where you should educate yourself more on the topic. I can only bring nudge you so far but if you believe that line coverage has anything to do with behavior testing I don't think you put any thought into verifying your work.

It becomes evident to me that you don't know at all why you made this change. I'm asking you a third time now: Why was the Badge changed but no test added? If you don't know what the code does then please revert it.

[oliviertassinari]

Impossible to collaborate on that premise since this argument applies anytime.

I think that we can default to testing less when we have a doubt. We have done this many times e.g. bugs in IE11, not worth it, it has been more efficient to have regressions and handle them. I think that it's best to wait for the pain to grow before doing something about it, we already have so many problems we know are important and need care.

Why was the Badge changed but no test added?

I have tried to answer it in #24253 (comment), the new generic test inside describeConformanceV5 doesn't support it. It's one rounding error compared to all the other components (144). Best to ignore it.

[eps1lon]

I'm sorry but you have the responsibility to explain the code you write. We have this expectation for any other contribution. What you did, expected behavior, actual behavior applies to everyone.