dbrgn / tracing-test

Access and evaluate tracing logs in async and sync tests.
Apache License 2.0
52 stars 26 forks source link

Add no-env-filter feature to disable log filtering #16

Closed threema-danilo closed 1 year ago

threema-danilo commented 2 years ago

Fixes #6.

teohhanhui commented 2 years ago

This doesn't seem to respect RUST_LOG as described here:

https://docs.rs/tracing-subscriber/latest/tracing_subscriber/fmt/index.html#filtering-events-with-environment-variables

Hardcoding to the equivalent of RUST_LOG=trace does not fix #6

threema-danilo commented 2 years ago

@teohhanhui see https://github.com/dbrgn/tracing-test/pull/12. I disagree that a commonly used env variable (with the purpose of configuring logging) should affect the outcome of an integration test.

However, if you want to control the filter using an env variable, you can use the no-env-filter crate feature in combination with custom logic in the logs_assert function. (Note: Right now the logs_assert function operates on string slices, I'd like to change that in the future to custom structs, which allow you to filter by log tag, severity, etc. However, that's not yet implemented.)

Hardcoding to the equivalent of RUST_LOG=trace does not fix https://github.com/dbrgn/tracing-test/issues/6

6 is about "allow tracing crates other than the current crate". This PR enables that.

threema-danilo commented 2 years ago

Note: A crate-level attribute that controls the default filter (globally) would also be a good improvement on top of this. PRs for that would be welcome!

teohhanhui commented 2 years ago

I disagree that a commonly used env variable (with the purpose of configuring logging) should affect the outcome of an integration test.

That's what upstream (tracing) has decided. Changing that behaviour in this crate that's supposed to just help set things up is counter-intuitive and unexpected.

threema-danilo commented 2 years ago

@teohhanhui I don't see how tracing has decided how the RUST_LOG env variable should have an impact on integration tests?

Anyways, your tone comes over as very demanding and unconstructive, bordering on rude. That may be a mis-interpretation on my side, but as the author of this crate I obviously decide what its goals are. And I am certain: I do not want my tests in CI to suddenly fail without any change in the code, just because the Docker image used introduced a new environment variable. This library is about asserting what's being logged, and for that the test needs to be able to control the logging configuration of the environment.

With this PR, you are free to read whatever environment variable you want, and implement your own filtering. If you have concrete ideas for improving this crate that are in line with its goals, I am happy to collaborate on a pull request. I have outlined some ideas above. And, to quote the license:

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.

Stargateur commented 1 year ago

but as the author of this crate

You use two github account ?

This library is about asserting what's being logged

Ah ! that not at all what I expect when I wanted this use this crate, I just want to debug the test not test the tracing. I must say I do not understand the goal to test the tracing of a crate that a little "too much" testing, tracing is about add debug, debug output change and is flexible that the very purpose, so for you the logging is someway a breaking change thing ? Or considered a bug if it's not there anymore ?

https://github.com/dbrgn/tracing-test/pull/12 mostly do what I personally wanted, for example I want to see the enter exit of function, but your PR https://github.com/dbrgn/tracing-test/pull/16 doesn't allow that.

I would be nice to have both view, so two macros, one for just debugging a test and one for assert tracing output.

PS: Oh https://crates.io/crates/test-log look like it's does more or less what I wanted.

I think the current name of this crate is misleading, I think a tracing-test should do what test-log do, and a tracing-assert do what actually tracing-test do Look like https://crates.io/crates/tracing-fluent-assertions is close to that

threema-danilo commented 1 year ago

You use two github account ?

Yes, this crate is partially developed on company time, and I use my work account for those commits.

Ah ! that not at all what I expect when I wanted this use this crate, I just want to debug the test not test the tracing.

Ah! That is indeed a different goal. I developed this library to allow testing certain implementation details of a service where – when an error is triggered – the service (intentionally) does not give out any error details to the caller. However, by adding log assertions, I can ensure that internally the error condition was properly handled. Using logs to test internal implementation details should be used very sparingly, but there are situations where it's very helpful. In that context, I definitely don't want env variables to influence my test results.

That's also the reason why the macro injects all those assertion errors – if you just wanted to debug your test, you wouldn't need those.

PS: Oh https://crates.io/crates/test-log look like it's does more or less what I wanted.

It does indeed 🙂

I think the current name of this crate is misleading

I don't think so, it's about testing the logs emitted by tracing. tracing-assert would have been an even better name, I agree, but I don't think it's worth renaming this crate. I'll however clarify the goals in the README.

threema-danilo commented 1 year ago

I'll however clarify the goals in the README.

I clarified the scope in the README and in the crate docs, and also linked to those other two crates you mentioned.