-
Notifications
You must be signed in to change notification settings - Fork 67
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
Added property tests for Algebra.Graph.Label #232
Conversation
Removed Num constraint for Show in Graph.Label.Minimum
Consider,
Clearly
|
Many thanks @adithyaov! This looks very nice. I've left a couple of minor comments. |
@adithyaov This doesn't currently pass CI. Could you push a fix please? |
Just restarted CI, perhaps we need to increase/remove Travis time limit for long-running commands? |
@adithyaov Very nice PR, many thanks! Just managed to find time for review and left a few minor comments. P.S.: CI has passed after restarting. |
@snowleopard Although |
@snowleopard Could you please restart Travis CI. I don't understand why it fails. As you suggested, increasing the time limit might work. |
@adithyaov I've restarted the CI. If you can find a way to make CI more reliable, please let me know how. |
@snowleopard I think this PR is ready for merge. Profiling might take a little more time. Can we safely ignore timeouts for the time being? |
@adithyaov Having reliable CI is very important since it helps a lot with reviews. This particular PR seems to push the testsuite too far, making CI unreliable, so I don't think we should merge it until we either get rid of slow tests in general, or slow tests in this particular PR that are responsible for slowing down the testsuite. |
@snowleopard Agreed. I'll try to complete profiling the tests as soon as possible. |
test/Algebra/Graph/Test/Label.hs
Outdated
test "StarSemiring" $ \(a :: Capacity Int) b c -> testStarSemiring a b c | ||
test "Dioid" $ \(a :: Capacity Int) b c -> testDioid a b c | ||
|
||
putStrLn "\n============ Minimum ============" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are actually testing Minimum (Path Int)
below, not Minimum
.
I think we should add tests for all types of labels we define in Algebra.Graph.Label
, perhaps, also adding some combinations like Minimum (Path Int)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps, also adding some combinations like Minimum (Path Int).
Perhaps you meant something else? Minimum (Path Int)
already exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant similar to Minimum Path
that you are already testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will do it in another PR. Some types (Optimum
) need not satisfy Semiring
yet can be used. I need to come up with a proper/better reasoning.
@adithyaov Thanks a lot for fixing the testsuite! I've added some comments. |
Label: 1. In label change (+) such that there is symmetry with (*) Test.Label 1. Change the placement of (//) annotations 2. Change string headers accordingly
1. Change CountShortestPaths e a to CountShortestPaths e 2. Add arbitrary instance for Optimum
@adithyaov Great work, thank you. Merged! |
The initial draft for
Algebra.Graph.Test.Label
.