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

Upgrade to MUnit 1.0.0-M6 #223

Merged
merged 15 commits into from
Aug 2, 2022

Conversation

armanbilge
Copy link
Member

@armanbilge armanbilge commented Jun 19, 2022

Closes #147.
Fixes #172.
Closes #208.

  • 974242f updates the build and removes CE2
  • ba30103 upgrades to MUnit 1.0.0-M5 source-compatibly, as demonstrated by unchanged tests
  • b58baef and beyond rework the tests to get off deprecated methods

There is currently failing test due to a regression in MUnit.

@armanbilge armanbilge changed the title Update to MUnit 1.0.0-M5 Upgrade to MUnit 1.0.0-M5 Jun 19, 2022
@armanbilge
Copy link
Member Author

There is currently failing test due to a regression in MUnit.

My bad, this is not a regression but a deliberate breaking change. See scalameta/munit#539.

.settings(
name := "munit-cats-effect",
Copy link
Member Author

Choose a reason for hiding this comment

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

We should make sure to register a Scala Steward migration for munit-cats-effect-3 -> munit-cats-effect.

@armanbilge
Copy link
Member Author

I think it would be great to start publishing milestones against this as soon as possible. There are quite a few improvements, and that way we can be sure that everything is working correctly.

@armanbilge
Copy link
Member Author

Gratefully tagging @valencik for review 😁

@@ -1,77 +1,24 @@
ThisBuild / tlBaseVersion := "1.0"
ThisBuild / tlBaseVersion := "2.0"
Copy link
Member

Choose a reason for hiding this comment

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

Are we really want to do a new major release based on the fifth milestone?..

Copy link
Member Author

Choose a reason for hiding this comment

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

Bumping the base version doesn't mean a new major release, it means we are starting the 2.x series of development that is not compatible with any 1.x release. I propose we publish this PR as 2.0.0-M1

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I just was confused with this reference to 2.0.0.

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha yes, we always debate the correct deprecation version for http4s too :) I don't really care, I will happily put whatever you want there.

scalaJSLinkerConfig ~= (_.withModuleKind(ModuleKind.CommonJSModule))
)

lazy val ce2 = crossProject(JSPlatform, JVMPlatform)
Copy link
Member

Choose a reason for hiding this comment

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

If we decided to make EOL the whole library ("munit-cats-effect-2"), I argue it should be described in docs. As well as changing munit-cats-effect-3 -> munit-cats-effect. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it should be described in docs. But if we make those changes in this PR, then it will go live as soon as it merges even before official release. Maybe should make a new branch for me to target my changes to, instead of main?

Copy link
Member

@danicheg danicheg Jun 22, 2022

Choose a reason for hiding this comment

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

I think we already can slice changes on removing CE2 support into another PR and merge it as soon as possible (because no things we should do after it) 👍🏻

Copy link
Member Author

Choose a reason for hiding this comment

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

Good plan, I will prepare it shortly.

Copy link
Member Author

Choose a reason for hiding this comment

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

@danicheg sorry, to clarify: do you mean slice the documentation changes, or do you mean slice 974242f specifically?

I don't think we should drop CE2 from the 1.x series.

Copy link
Member

Choose a reason for hiding this comment

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

Aww. Probably I was wrong here. I was thinking we want to deprecate the whole library. Because we integrate CE support to MUnit, not vice versa. If we encourage investing in migration to CE3, let's do it at max 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha, I see. Actually that was how this library started but it is better it remains independent. Origin story:

Copy link
Member

Choose a reason for hiding this comment

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

Okay, but why not deprecate the CE2-based library? Speaking as one of the http4s maintainers, it'd be totally fine. Because I'm confident, here we integrate precisely CE support.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should just EOL 1.x when 2.x is released. And 2.x won't support CE2 at all.

Copy link
Member

@valencik valencik left a comment

Choose a reason for hiding this comment

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

Wow this really simplifies things! Considerably less special casing for ScalaJS which is cool.

I also published this locally and tested that one of my favourite fixes to munit v1.0, filtering suites no longer runs their fixtures, works as expected: valencik/munit-fixture-suite-filtering#1.

Thanks a bunch for writing this, I'm excited to use it :)

@kpodsiad
Copy link

Can't wait to use this, thank you for your effort @armanbilge!

@armanbilge armanbilge changed the title Upgrade to MUnit 1.0.0-M5 Upgrade to MUnit 1.0.0-M6 Jun 28, 2022
@armanbilge armanbilge requested a review from danicheg July 16, 2022 13:27
@armanbilge
Copy link
Member Author

Considerably less special casing for ScalaJS which is cool.

Thanks for mentioning this. Actually Scala.js shouldn't need any special casing now, there was some dead code I forgot to delete :)

@matthughes
Copy link

Is there anything holding up this PR being merged? And does anyone know the 1.0.0 release plans of Munit?

@armanbilge
Copy link
Member Author

I'm going to merge and release a milestone.

@armanbilge armanbilge merged commit f7d4c2b into typelevel:main Aug 2, 2022
@armanbilge
Copy link
Member Author

@matthughes https://github.com/typelevel/munit-cats-effect/releases/tag/v2.0.0-M1

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.

Lack of Cross-Platform Support for ResourceSuiteLocalFixture Integrate with upcoming munit v1
5 participants