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

Strictness tests for Map construction #1021

Merged
merged 1 commit into from
Oct 31, 2024

Conversation

meooow25
Copy link
Contributor

@meooow25 meooow25 commented Aug 11, 2024

For #1019.

Adds a bunch of property tests that try to create strict and lazy Maps with bottom values and check if the result map is bottom.

We also need to do IntMap but this PR is already huge, will do that later.

@meooow25
Copy link
Contributor Author

Tests should pass once #473 and #1022 are fixed.

@meooow25 meooow25 force-pushed the map-construct-strictness-tests branch 2 times, most recently from e1017c8 to 3cd7e25 Compare August 18, 2024 15:11
@meooow25
Copy link
Contributor Author

Alright, this is good to go.
Dunno if you want to take a look at this @treeowl.

Copy link
Contributor

@treeowl treeowl left a comment

Choose a reason for hiding this comment

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

I haven't read it all yet, but I have a few thoughts.

prop_strictFromSet fun set =
isBottom (M.fromSet f set) === any (isBottom . f) (Set.toList set)
where
f = coerce (applyFunc fun) :: OrdA -> A
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the explicit coerce here reduces clarity without (at least meaningfully) improving test performance. Less importantly: I think Bot A is overkill; Bot () should do the trick. That change should improve test performance by reducing the number of random numbers generated.

Copy link
Contributor Author

@meooow25 meooow25 Aug 20, 2024

Choose a reason for hiding this comment

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

I think the explicit coerce here reduces clarity without (at least meaningfully) improving test performance.

In all of the tests with functions, the point is to get some X -> Y where Y may be bottom. This is done by generating Func X (Bot Y) and using coerce . applyFunc on it. It is true that some tests would work without the coerce, but I don't see a good reason to deviate from the standard pattern.

Less importantly: I think Bot A is overkill; Bot () should do the trick. That change should improve test performance by reducing the number of random numbers generated.

This is true for this test, but again there is little reason to deviate. The tests run fast and I don't think it is worth trying to optimize tests case-by-case unless it is really causing a problem.

containers-tests/tests/Utils/Strictness.hs Show resolved Hide resolved
containers-tests/tests/map-strictness.hs Show resolved Hide resolved
@meooow25
Copy link
Contributor Author

@treeowl do you have more comments on this?

@meooow25
Copy link
Contributor Author

@treeowl are you still reviewing this?

I would like to get the testing PRs submitted soon because they end up blocking other stuff (#1042, #963, similar IntMap tests, etc).

@meooow25
Copy link
Contributor Author

I'll check this once for problems and merge if it seems fine.

@meooow25 meooow25 force-pushed the map-construct-strictness-tests branch from 38a9c14 to 53f6be5 Compare October 31, 2024 05:38
This aims to reduce the chance of introducing strictness bugs.
Since we use the same Map type for lazy and strict maps, it is not
possible to ensure appropriate strictness at the type level. So we turn
to property tests.

Arbitrary Set and Map generation is moved from set-properties.hs and
map-properties.hs to ArbitrarySetMap.hs to be shared with the new
strictness tests.
@meooow25 meooow25 force-pushed the map-construct-strictness-tests branch from 53f6be5 to b6fee41 Compare October 31, 2024 22:50
@meooow25
Copy link
Contributor Author

Alright, it's looking good.

@meooow25 meooow25 merged commit 7e7ce15 into haskell:master Oct 31, 2024
12 checks passed
@meooow25 meooow25 deleted the map-construct-strictness-tests branch October 31, 2024 23:22
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.

2 participants