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

Upgraded specs2 to 5.0.0-RC-07 for Scala3 #207

Closed
wants to merge 2 commits into from

Conversation

etorreborre
Copy link

@etorreborre etorreborre commented Sep 8, 2021

and reformatted the fragments a bit to look better when using Scala 3.

This PR prepares discipline-specs2 for the upcoming specs2-5.0.0 release.

and reformatted the fragments a bit to look better
when using Scala 3
@vasilmkd
Copy link
Member

vasilmkd commented Sep 8, 2021

This turned out to be easier than expected?

@etorreborre
Copy link
Author

Well, unless I missed something :-)

@vasilmkd
Copy link
Member

vasilmkd commented Sep 8, 2021

This looks good to me, but I will wait for another review from @djspiewak and @armanbilge.

@vasilmkd
Copy link
Member

vasilmkd commented Sep 8, 2021

The final thing to consider is whether to release stable versions that depend on release candidates.

@etorreborre
Copy link
Author

I propose to wait a bit until I publish the final specs2-5.0.0. I need to address the cats-effect-testing library first.

Comment on lines +117 to 119
"org.specs2" %%% "specs2-scalacheck" % specs2DottyV
else
"org.specs2" %%% "specs2-scalacheck" % specs2V
Copy link
Member

Choose a reason for hiding this comment

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

This makes me really nervous because it makes it very hard for users to easily cross-build between Scala 2 and Scala 3.

Copy link
Member

Choose a reason for hiding this comment

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

@rossabaker suggested in #192 (comment) that the next version of discipline-specs2 is Scala 3 only.

Copy link
Member

Choose a reason for hiding this comment

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

That's probably reasonable. It'll be more of a problem for cats-effect-testing though

@armanbilge
Copy link
Member

The final thing to consider is whether to release stable versions that depend on release candidates.

@vasilmkd would you mind publishing a snapshot so I can test this downstream? :)

@vasilmkd
Copy link
Member

vasilmkd commented Sep 8, 2021

@armanbilge

Snapshot release will be available soon.

"io.vasilev" %%% "discipline-specs2" % "1.2-7-e3ce260"

@armanbilge
Copy link
Member

Awesome, thanks! Trying now.

@armanbilge
Copy link
Member

Cool, works in https://github.com/armanbilge/cheshire/runs/3546471023. Trying another one of my projects next.

@armanbilge
Copy link
Member

Update, also working in https://github.com/armanbilge/schrodinger/runs/3547259110. That's more interesting maybe because it tests monad transformers against CE3 laws.

@etorreborre I just realized the discipline output is sometimes missing newlines, e.g. https://github.com/armanbilge/cheshire/runs/3546471023#step:6:70

@etorreborre
Copy link
Author

@armanbilge thanks I thought I had tested that but I must have missed the mutable spec case. I will fix it.

@etorreborre
Copy link
Author

Actually I'm not sure if there's a meaningful fix in discipline-specs2. The easiest thing, if you are using a mutable spec is to intersperse some brs in between the checkAll calls:

class MutableDisciplineSpec extends Specification with Discipline {
  checkAll("Int1", RingLaws.ring); nl
  checkAll("Int2", RingLaws.ring); nl
  checkAll("Int3", RingLaws.ring)

  def nl = { br; br }
}

I tried to write a display function doing this interspersing but given the amount of side effects in a mutable spec this is close to impossible!

The simplest thing is actually to use a non-mutable spec:

class DisciplineSpec extends Specification with Discipline {
  def is =
      checkAll("Int1", RingLaws.ring) ^
      checkAll("Int2", RingLaws.ring) ^
      checkAll("Int3", RingLaws.ring)
}

@armanbilge
Copy link
Member

@etorreborre thanks for looking into this so quickly. Apologies if I'm missing something, but can't the nl just be baked into the checkAll method?

@etorreborre
Copy link
Author

If you do it then you have an extra nl at the end of the spec even if there's only checkAll being called. If that's ok with you, you can define:

def check(name, prop) = { checkAll(name, prop); br; br }

Having some sort of wysiwig display in specs2 is quite hard, either when side-effects are involved with a mutable spec or with a s2""" ... """ string in an immutable spec (to respect newlines and whitespace). I tried to improve the situation in specs2-5.x but there are definitely some corner cases that will pop-up.

@armanbilge
Copy link
Member

If you do it then you have an extra nl at the end of the spec even if there's only checkAll being called.

IMO we should make that change in this PR. An extra nl is preferable to I suspect nearly every downstream having to account for this one way or another. Thoughts @djspiewak @vasilmkd?

@etorreborre
Copy link
Author

I prefer to be able to control exactly how things are displayed but the user is king :-).

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.

5 participants