Glasscat tests #152
Glasscat tests #152
Conversation
these should be covered in test/testsets/JuliaLang.jl
Codecov Report
@@ Coverage Diff @@
## main #152 +/- ##
==========================================
+ Coverage 53.66% 56.25% +2.59%
==========================================
Files 66 66
Lines 6472 6332 -140
==========================================
+ Hits 3473 3562 +89
+ Misses 2999 2770 -229
Continue to review full report at Codecov.
|
| @@ -338,170 +338,3 @@ function info(io::IO, glass::Glass) | |||
| end | |||
|
|
|||
| info(g::AbstractGlass) = info(stdout, g) | |||
|
|
|||
There was a problem hiding this comment.
is the docstring function being permanently deleted or did you just move it somewhere? Even though we're not using it much now it seems to be functioning ok. Other people might find it useful for making tools to analyze glasses. Is it necessary to remove it?
There was a problem hiding this comment.
I took it out because it seemed like duplicate code (I can't remember where from), it was unused, and it was lowering our test coverage.
Just had a look and it seems to be essentially the same as info() above but without the padding. We should really have a third function to handle the data extraction such that info() and docstring() only need to handle the string formatting.
The other thing that confused me was that we weren't even using docstring() to generate the docstrings for GlassCat - we had an abbreviated version seen in generate.jl > glassinfo_to_docstring. This is essentially a triple duplicate - what do you think we should keep?
There was a problem hiding this comment.
Just had a look and it seems to be essentially the same as info() above but without the padding
This is basically what it did - just printed the same stuff as info with correct formatting for documentation generation - I don't see much need for it if we don't have glass docs any more.
I think glass catalog jl files get written with short form information for tooltips in VS Code, this function was used specifically for long form information for use in generation of the documentation website
There was a problem hiding this comment.
@BrianGun you can have the final say - if you want to keep the code then feel free to git revert d3197a2
e.g. CARGILLE instead of GlassCat.CARGILLE
alfredclwong
force-pushed
the
glasscat-tests
branch
from
c54eb8c to
eb8016a
Compare
test/testsets/GlassCat.jl
Outdated
|
|
||
| # TODO @test glassnames() | ||
|
|
||
| # !! TODO !! for some reason uncommenting this test causes the alloc tests at L145-146 to fail |
There was a problem hiding this comment.
This is super weird... Have you tried defining it differently? e.g. define a separate filter function
There was a problem hiding this comment.
i'll try that out this morning as well as anything else i can think of.
edit: i doubt changing the filter function will do anything - x -> false still causes failures. i'm now trying to comment out bits of findglass to see if that's causing the issue
might be worth asking for outside help with this one. @BrianGun have you got any ideas?
There was a problem hiding this comment.
After some digging i've found that the issue is caused by calling a function which takes another function as an argument, such as function findglass(condition::Function). Even if findglass does absolutely nothing, this still causes the issue.
We need to fix the allocations test call to run within a local scope to avoid allocations from accessing variables. https://discourse.julialang.org/t/using-allocated-to-track-memory-allocations/4617/5
I'll add the wrappedallocs macro from https://github.com/JuliaAlgebra/TypedPolynomials.jl/blob/master/test/runtests.jl to our runtests.jl. We should use this to preemptively "fix" any other benchmarks we are running.
|
@BrianGun I think these changes are ready for approval, if you're happy with them. The wrapped allocations fix is worth a read for future reference. |
Improve GlassCat test coverage and introduce
wrappedallocmacro to avoid issues with counting allocations in global scope.