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

Monkey patch Test.scrub_backtrace #287

Merged
merged 5 commits into from
Oct 2, 2023
Merged

Monkey patch Test.scrub_backtrace #287

merged 5 commits into from
Oct 2, 2023

Conversation

oxinabox
Copy link
Member

Problem is
the @test macro is that it scrubs backtraces to remove stuff from deeper down the stack than the location of the macro, and since test_rrule etc use those macros directly the back trace no longer includes the call site in the users tests.

Simple solution is to just stop scrubbing backtraces entirely.
If people like this we could follow up with something more sophisticated.
But lets have that as a seperate PR

This does mean whenever tests are run this warning will show.

     Testing Running tests...
WARNING: Method definition scrub_backtrace(Any, Any, Any) in module Test at /home/oxinabox/dist/julia-cedar/vanilla/latest/share/julia/stdlib/v1.11/Test/src/Test.jl:89 overwritten at /home/oxinabox/JuliaEnvs/ChainRulesTestUtils.jl/src/ChainRulesTestUtils.jl:26.
Testing ChainRules.jl

Could surpress that though.

@codecov
Copy link

codecov bot commented Sep 28, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (c421f7c) 93.65% compared to head (b7f5b0a) 93.50%.

❗ Current head b7f5b0a differs from pull request most recent head ebfe4c1. Consider uploading reports for the commit ebfe4c1 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #287      +/-   ##
==========================================
- Coverage   93.65%   93.50%   -0.16%     
==========================================
  Files          12       12              
  Lines         347      354       +7     
==========================================
+ Hits          325      331       +6     
- Misses         22       23       +1     
Files Coverage Δ
src/ChainRulesTestUtils.jl 88.88% <87.50%> (-11.12%) ⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/ChainRulesTestUtils.jl Outdated Show resolved Hide resolved
@oxinabox
Copy link
Member Author

oxinabox commented Sep 29, 2023

I tried adding a with_logger(NullLogger() around the @eval and it didn't silence the warning.
:-(

Nor did redirect_stdout(devnull) or redirect_stderr(devnull)

Suppressor.jl did get it though

Copy link
Member

@willtebbutt willtebbutt left a comment

Choose a reason for hiding this comment

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

LGTM. Tested locally on some of my own code, and seems to work fine. Play nicely with Revise etc (as you would expect), so seems to be fine.

src/ChainRulesTestUtils.jl Outdated Show resolved Hide resolved
@oxinabox oxinabox merged commit 49a0324 into main Oct 2, 2023
15 of 16 checks passed
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.

3 participants