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

Restore v0.7.x fixture ordering #724

Merged
merged 2 commits into from
Jan 2, 2024
Merged

Restore v0.7.x fixture ordering #724

merged 2 commits into from
Jan 2, 2024

Conversation

valencik
Copy link
Collaborator

@valencik valencik commented Dec 13, 2023

⚠️ Breaking Change ⚠️

This PR changes the ordering in which suite-local fixture afterAll methods are called.
It implements the trick @olafurpg describes in #573 (comment) to restore the fixture ordering of v0.7.x
This ordering was changed in the first v1.0 milestone release
Therefore it is a breaking change within the v1.0 milestones in order to avoid a breaking change between v0.7.x and v1.0 overall.

Namely, we split up the suite-local fixture implementation into two fixtures, one of the before* methods, and one for the after* methods. Then we explicitly order munitFixtures to have the befores up front and the afters at the end.

Of particular note, the tests/shared/src/main/scala/munit/FixtureOrderFrameworkSuite.scala file now looks the same as it did before https://github.com/scalameta/munit/pull/430/files?diff=split&w=0#diff-714f7ef13dfbbd4747770936315606f95d8aeba5f24890a45a4be216cda9d8a1

Note: The v0.7.x style ordering is not to run the after* methods in opposite order to how the before* methods were run, making a mirror image like pattern. Instead it's more like after* methods run in the same order as before* methods except for suite-local fixtures which are special cased to run last.

We split up the suite-local fixture implementation into two fixtures,
one of the `before*` methods, and one for the `after*` methods.
Then we explicitly order `munitFixtures` to have the befores up front
and the afters at the end.
@valencik valencik self-assigned this Dec 13, 2023
@valencik valencik marked this pull request as ready for review December 13, 2023 03:41
@@ -34,7 +34,7 @@ class SuiteLocalFixtureSuite extends FunSuite {
}

override def afterAll(): Unit = {
assertEquals(counter(), 17)
assertEquals(counter(), -10)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

Looks good to me, especially if we can keep people from being surprised by the change when going from 0.

For me a more natural way would be a complete reverse order, but that would be another breakage, so let's just go with the first approach.

Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

LGTM. At this point, the 1.x series has been in milestone phase for so long that this is almost a breaking change. It's fine IMO to keep the old behavior or merge this PR as long as we communicate clearly how to get the desired ordering in the release notes.

@tgodzik tgodzik merged commit 0d20dbc into main Jan 2, 2024
9 checks passed
@tgodzik tgodzik deleted the revert-fixture-ordering branch January 2, 2024 18:21
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.

4 participants