search

scalameta / munit

Scala testing library with actionable errors and extensible APIs
https://scalameta.org/munit
Apache License 2.0
428 stars 88 forks source link

Extract diff module #756

Closed majk-p closed 4 months ago

majk-p commented 5 months ago

This PR aims to extract diff calculation to a separate module to resolve https://github.com/scalameta/munit/issues/745

Since diff had to share a bit of code with the rest of munit, I've also extracted munit-core that provides the minimal common part.

majk-p commented 5 months ago

@valencik @tgodzik this change breaks mima on purpose due to reorganisation of modules. Please advise what's your preferred strategy to deal with mima check considering 1.0.0 is underway.

tgodzik commented 5 months ago

@valencik @tgodzik this change breaks mima on purpose due to reorganisation of modules. Please advise what's your preferred strategy to deal with mima check considering 1.0.0 is underway.

Extracting munitDiff will break mima anyway. It's probably the last chance to do it. We should work on actual release later on. Any objections?

valencik commented 5 months ago

@valencik @tgodzik this change breaks mima on purpose due to reorganisation of modules. Please advise what's your preferred strategy to deal with mima check considering 1.0.0 is underway.

I first want to note that the ecosystem is eager for a 1.0 release. It is not ideal to be on milestones this long. And we have a split of projects on 0.7.x and 1.0.x milestones throughout the ecosystem.

However, I wanted to at least bump us to the latest Scala 3 LTS before cutting 1.0. Unfortunately, we're blocked by https://github.com/scala/scala3/issues/19594 not having yet been ported back to a LTS version.

All this to say, I don't have strong objections to the plan here.

tgodzik commented 4 months ago

You might want to rebase the PR to see if it passes.

majk-p commented 4 months ago

After brainstorming with @zainab-ali and inspired by this commit I did an overhaul of this change. Most of the Printers functionality remains in munit while only logic for string cleanup is extracted to diff (since it's the only used part). I think it makes things cleaner and we no longer have to deal with disentangling Clues.

tgodzik commented 4 months ago

Looks like the compilation is failing, could you take a look?

majk-p commented 4 months ago

I just run compile and test locally and everything worked well. I cannot see the CI logs in actions :disappointed:

zainab-ali commented 4 months ago

Thanks @majk-p and @tgodzik, the PR itself looks great!

Unfortunately the rencent changes to munit include a Native upgrade to 0.5. Weaver has a way to go until moving to 0.5 itself - see the discussion for cats-effect.

I'm brainstorming approaches we could take. Would it be viable to release this on 0.4.x too?

tgodzik commented 4 months ago

I can do that manually later on. Hopefully, this time it will work reasonably.