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

Make flaky tag work with Scalacheck suites #478

Merged
merged 2 commits into from
Jan 17, 2022

Conversation

olafurpg
Copy link
Member

@olafurpg olafurpg commented Jan 6, 2022

Previously, the Scalacheck suite transform was registered after the
.flaky transform so a flaky test would fail even if it was OK to
ignore flaky failures. Reordering the transforms fixes the issue.

Previously, the Scalacheck suite transform was registered after the
`.flaky` transform so a flaky test would fail even if it was OK to
ignore flaky failures. Reordering the transforms fixes the issue.
@olafurpg olafurpg requested a review from gabro January 6, 2022 18:03
@@ -29,7 +29,7 @@ trait ScalaCheckSuite extends FunSuite {
implicit def unitToProp(unit: Unit): Prop = Prop.passed

override def munitTestTransforms: List[TestTransform] =
super.munitTestTransforms :+ scalaCheckPropTransform
scalaCheckPropTransform +: super.munitTestTransforms
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like the discussions around transform registrations paid off big time. I was a big proponent for side-effecting transforms but that would have been a nightmare to debug for this bug here.

Copy link
Member

@gabro gabro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM🙏

@gabro
Copy link
Member

gabro commented Jan 6, 2022

Mm the CI failure looks legit

There was a bug in the JS/Native implementation of `RunNotifier` so that
it would report that a test was both ignored and that it succeeded.
@olafurpg
Copy link
Member Author

The test failure was a legitimate bug, which only got surfaced now because of the change. The problem was that the JS/Native test reporter reported that ignored tests also succeeded.

@olafurpg olafurpg enabled auto-merge (squash) January 17, 2022 08:28
@olafurpg olafurpg merged commit 9653a0e into scalameta:main Jan 17, 2022
@olafurpg olafurpg deleted the flaky-scalacheck branch January 17, 2022 08:28
olafurpg added a commit to olafurpg/munit that referenced this pull request Jan 17, 2022
olafurpg added a commit to olafurpg/munit that referenced this pull request Jan 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants