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] Karma should fail if errors are thrown #543

Merged
merged 3 commits into from
Nov 6, 2020

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Nov 6, 2020

I have based the pull request on top of the two wrong commits that were reverted. The expected outcome is that the CI to be failing

A post-mortem

  • Everything starts with [DataGrid] Add fluid columns width support #480. The pull request is depending on a week old commit. Damien pushed some refactorization around the state management that [DataGrid] Add fluid columns width support #480 didn't include when running the tests (based on a week old commit). Once we merged, the changes conflicts and broke one of the tests in the CI (on master).
  • As a forward-looking pragmatic approach, we started looking into how we could fix the issue (instead of reverting). Damien made a pull request Fix columns #542, probably based on a gut feeling that the logic should be written in a different way.
    Damien got a positive feedback loop: the tests are now green!
  • 💥 Looking closer, it turns out that. The feedback loop was plain wrong. The new logic was introducing an error (setTimeout) that was preventing all the tests in the CI to run. Basically, only the first test is run, instead of the whole suite.
  • I revert the last two commits, we have made things worse. Time to hit the reset button.

Before
before

After exit code: 0 🙀
after

Learnings

Capture d’écran 2020-11-06 à 13 47 23

Next step

@oliviertassinari oliviertassinari marked this pull request as ready for review November 6, 2020 12:27
@oliviertassinari oliviertassinari force-pushed the ci-smooke-test branch 3 times, most recently from 4ada4a5 to a21faff Compare November 6, 2020 12:28
@oliviertassinari oliviertassinari changed the title [test] Smoke test [test] Karma should fail if errors are thrown Nov 6, 2020
@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Nov 6, 2020
test/karma.conf.js Outdated Show resolved Hide resolved
test/karma.conf.js Outdated Show resolved Hide resolved
Co-authored-by: damien tassone <damien.tassone@gmail.com>
@oliviertassinari oliviertassinari merged commit e3ab096 into mui:master Nov 6, 2020
@oliviertassinari oliviertassinari deleted the ci-smooke-test branch November 6, 2020 19:37
oliviertassinari added a commit to oliviertassinari/material-ui that referenced this pull request Nov 7, 2020
The issue was found in mui/mui-x#543.
I also create a new setupKarma.js file so the X repo can import it directly.
dtassone pushed a commit to dtassone/material-ui-x that referenced this pull request Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants