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

Add tests for @time compilation reporting #45130

Merged
merged 6 commits into from
May 12, 2022

Conversation

IanButterworth
Copy link
Sponsor Member

These tests are based off the examples in SnoopCompile's manual https://timholy.github.io/SnoopCompile.jl/stable/snoopr/

@IanButterworth
Copy link
Sponsor Member Author

@vtjnash I'd appreciate a 2nd set of eyes on this, especially the stdout capture test macro

@vtjnash
Copy link
Sponsor Member

vtjnash commented May 4, 2022

Pipe, and especially readavailable, are not reliable or safe to use for capturing stdout

@vtjnash
Copy link
Sponsor Member

vtjnash commented May 4, 2022

Is this something supported by @test_warn?

@IanButterworth
Copy link
Sponsor Member Author

Is this something supported by @test_warn?

Unfortunately not because @time outputs to stdout. Perhaps there should be @test_stdout and @test_stderr, and @test_warn could be an alias for the latter?

@IanButterworth
Copy link
Sponsor Member Author

How about using a file? (pushed)

@IanButterworth
Copy link
Sponsor Member Author

bump @vtjnash

test/misc.jl Outdated Show resolved Hide resolved
Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

yeah, an IOStream should be fine, but let's use mktemp so it gets cleaned up immediately too

Co-authored-by: Jameson Nash <[email protected]>
@IanButterworth IanButterworth added the status:merge me PR is reviewed. Merge when all tests are passing label May 12, 2022
@IanButterworth IanButterworth merged commit b33e64e into JuliaLang:master May 12, 2022
@giordano giordano removed the status:merge me PR is reviewed. Merge when all tests are passing label May 13, 2022
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.

None yet

3 participants