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

make TestLogger respect maxlog #43641

Merged
merged 3 commits into from
Jan 7, 2022
Merged

Conversation

ericphanson
Copy link
Contributor

@ericphanson ericphanson commented Jan 2, 2022

Fixes #41625, borrowing some code from SimpleLogger and ConsoleLogger.

Note that this still doesn't work:

julia> @test_logs (:error, "an error") erronce()

julia> @test_logs erronce()

since @test_logs creates a fresh logger each time (so they don't have a shared count for maxlog).

But at least there is a way to test maxlog logging statements (as shown in the diff, using Test.collect_test_logs). And Test.collect_test_logs is possibly part of the public API since it is referenced from @test_logs? Although it doesn't have a docstring and isn't in the manual.

Edit: actually a better way to test this using only @test_logs is just:

@test_logs (:error, "an error") (erronce(); erronce())

This errors on master but passes on this branch.

@DilumAluthge DilumAluthge requested a review from c42f January 3, 2022 00:11
@vtjnash vtjnash merged commit 02f7332 into JuliaLang:master Jan 7, 2022
@vtjnash
Copy link
Member

vtjnash commented Jan 7, 2022

This could perhaps break someone's test, but it seems unlikely. I think generally we have chosen not to parse the arguments in the TestLogger, so that the test has a true precise record of what was called (for example, we also ignore the minlevel). But it sounds like #41625 has support for changing this particular behavior.

@ericphanson ericphanson deleted the eph/test-logger branch January 7, 2022 23:18
@c42f
Copy link
Member

c42f commented Jan 9, 2022

I think generally we have chosen not to parse the arguments in the TestLogger, so that the test has a true precise record of what was called

Yes, I think this change is a bit questionable in that respect — perhaps a neater strategy would be to enhance the pattern matching in @test_logs rather than modify which log events are recorded. This (edit: ie, the current PR) is easier though...

@ericphanson
Copy link
Contributor Author

perhaps a neater strategy would be to enhance the pattern matching in @test_logs rather than modify which log events are recorded.

What would you suggest?

Alternatively, I wonder how much one can really access the log events recorded via the public API- perhaps exposing a richer interface would let users test things like maxlog without needing special handling here. I’ve found @test_logs a bit difficult to use compared to Test.collect_test_logs but I don’t think that’s part of the public interface.

ericphanson added a commit to ericphanson/julia that referenced this pull request Feb 8, 2022
@ericphanson
Copy link
Contributor Author

for example, we also ignore the minlevel

@vtjnash that's not true, right?

Logging.min_enabled_level(logger::TestLogger) = logger.min_level

julia> Test.collect_test_logs() do
       @debug "hi"
       end
(Test.LogRecord[], nothing)

@vtjnash
Copy link
Member

vtjnash commented Feb 8, 2022

Ah, apparently I was wrong

@ericphanson ericphanson mentioned this pull request Feb 8, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 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.

TestLogger does not respect maxlog
3 participants