-
Notifications
You must be signed in to change notification settings - Fork 4
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
put test objects in implements and test modules #37
Conversation
f9dc2ec
to
0737cc7
Compare
Codecov Report
@@ Coverage Diff @@
## main #37 +/- ##
==========================================
- Coverage 82.38% 75.00% -7.39%
==========================================
Files 5 5
Lines 159 176 +17
==========================================
+ Hits 131 132 +1
- Misses 28 44 +16
📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
else | ||
@implements IterationInterface{:indexing} NamedTuple [(a=1, b=2, c=3, d=4)] # No reverse on 1.6 | ||
end | ||
# @implements IterationInterface{(:reverse,:indexing)} String |
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.
What's up with the commented ones?
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.
They weren't actually tested before anyway 😅
Good reason for the PR in itself. Guess I should make some test data for them...
s = Base.IdSet(); push!(s, "a"); push!(s, "b") | ||
@test Interfaces.test(SetInterface, Base.IdSet, [s]) | ||
end | ||
@test_broken Interfaces.test(ArrayInterface, Base.LogicalIndex, [to_indices([1, 2, 3], ([false, true, true],))[1]]) |
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.
Is this fixable in Base Julia?
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.
Yes by putting collect(LogicalIndex(I))
here
But of course that has a serious performance hit.
This PR moves test objects back to the
@implements
macro, and implements thetest
method for Module:Which cuts a huge amount of code from BaseInterfaces.jl and ensures everything implemented is tested.
Closes #35