added ITF1788 tests #505
added ITF1788 tests #505
Conversation
Codecov Report
@@ Coverage Diff @@
## master #505 +/- ##
==========================================
- Coverage 90.94% 90.10% -0.84%
==========================================
Files 25 25
Lines 1789 1789
==========================================
- Hits 1627 1612 -15
- Misses 162 177 +15
Continue to review full report at Codecov.
|
I find this as a very clever solution!
Good point!
OK
OK @lucaferranti I'll try to review this PR in detail this afternoon... busy morning... |
There was a problem hiding this comment.
I've left some comments, some of them for future development. The main message is: let's get JuliaIntervals/IntervalContractors.jl#48 merged, and then merge this one, after updating the rev tests.
| @@ -0,0 +1,162 @@ | |||
| @testset "minimal_overlap_test" begin | |||
|
|
|||
| @test_broken MISSING_overlap(emptyinterval(), emptyinterval()) === bothEmpty | |||
There was a problem hiding this comment.
For #504, I'm coding the overlap function; my idea is to return a Symbol, such as :both_empty or :before. Does it make sense to change this file accordingly? (Sorry for the late request.)
There was a problem hiding this comment.
good point. I can change it, an alternative would be to implement the overlap states with @enum. Not sure which one is better 🤔
Btw, since the overlap is fairly independent by the rest of the work, I think it would make more sense to have it as a stand-alone PR to master, maybe continuing from #266 .
There was a problem hiding this comment.
I didn't know about #266, and it LGTM. There are minor differences with respect to bb4d665 though, the major being that it returns Strings instead of a Symbol; I would favor the second one. (Your proposal with @enum may be even better, for performance if overlap is used in the actual code.)
If you think #266 is worth merging, go ahead; #271 (and $504) will have to incorporate it.
There was a problem hiding this comment.
I'll have a look at #266 tomorrow or on Tuesday. If you think it's a nice idea I could try to build on top of that with @enum (if you haven't done it already)
|
Everything looks good to me. One remark: would it be feasible to add ITF1788.jl as test target too and just run the tests from this package without having to copy the files over here? |
so far I've generated the tests directly into IA once I've fixed something, so that I don't have to copy paste. Some better automation might be cool but generating the tests on the PR could be a little heavy? Anyways, better documentation in ITF1788.jl is for sure needed, I'll update that tomorrow :D |
|
What I was thinking is that it feels like that kind of change would be better examinated by looking at the change in the generator. So it may make more sense to discuss them in ITF1788.jl and just consider here that we trust our beloved dependency ^^' |
@lbenet , we met today with @Kolaru and agreed on some first step to get things going. This PR adds *all the ITF1788 tests, including the currently failing ones, which are marked as broken. This way we can keep track of what is currently working, what isn't and avoid regression while gradually adding #271
Kolaru will then strip 271 of everthing that is breaking or "controversial" so that the PR effectively restructures the package and can be easily merged. Next we can discuss what is to discuss and keep working through smaller independent PRs.
Regarding this PR:
IntervalContractorsis added as test target, whether merging it to IA or not is left for further discussion@test_brokeninstead of@test_skip, as it makes it easier to see what has been fixed as one fixes testslibieeep1788_class.jlstill use@test_skipbecause the code was failing to parse some stringsBtw, I also enabled wikis in the repository and created this one to track discussions and meetings etc. I think it's better to have long discussions and meeting minutes there, but let's see how it goes. If you don't like it, I can remove it.