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

Add OrderTests for SortedSet #2915

Merged
merged 1 commit into from
Jun 27, 2019
Merged

Add OrderTests for SortedSet #2915

merged 1 commit into from
Jun 27, 2019

Conversation

jooohn
Copy link
Contributor

@jooohn jooohn commented Jun 27, 2019

I found we have PartialOrderTests for SortedSet, but not OrderTests.

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@207bb0d). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2915   +/-   ##
=========================================
  Coverage          ?   94.26%           
=========================================
  Files             ?      364           
  Lines             ?     6952           
  Branches          ?      188           
=========================================
  Hits              ?     6553           
  Misses            ?      399           
  Partials          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 207bb0d...dab66fb. Read the comment docs.

@@ -19,6 +19,8 @@ class SortedSetSuite extends CatsSuite {
checkAll("Semigroupal[SortedSet]", SerializableTests.serializable(Semigroupal[SortedSet]))

checkAll("SortedSet[Int]", FoldableTests[SortedSet].foldable[Int, Int])
checkAll("Order[SortedSet[Int]]", OrderTests[SortedSet[Int]].order)
checkAll("Order.reverse(Order[SortedSet[Int]])", OrderTests(Order.reverse(Order[SortedSet[Int]])).order)
checkAll("PartialOrder[SortedSet[Int]]", PartialOrderTests[SortedSet[Int]].partialOrder)
Copy link
Contributor

Choose a reason for hiding this comment

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

@kailuowang Do we still need these?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is redundant, but we are not very consistent when it comes testing laws already covered by the type class laws lower in the hierarchy. That's why I didn't raise an objection here. There is benefit in redundant tests, also sometimes it's hard to track which tests are already covered and remove them. Reducing redundancy to minimum (i.e. always test only the laws lowest in the hierarchy) might have its own problem as well. So right now we are doing it rather arbitrarily. We should probably come up with a principle and add it to the guideline. I created an issue for it #2919

@kailuowang kailuowang merged commit e2c255e into typelevel:master Jun 27, 2019
@kailuowang kailuowang added this to the 2.0.0-RC1 milestone Aug 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants