rust-lang / log

Logging implementation for Rust
https://docs.rs/log
Apache License 2.0
2.12k stars 248 forks source link

adding line_number test + updating some testing infrastructure #619

Closed DIvkov575 closed 3 months ago

DIvkov575 commented 4 months ago

resolves #601

DIvkov575 commented 4 months ago

I think that the log method is not actually being called. I attempted to add various debug statements into the log method of the logger, and the debugging statements did not execute.

Could somebody please let me know if I am creating a logger instance incorrectly?

DIvkov575 commented 4 months ago

@Thomasdezeeuw Is it ok for me to ping you for help?

Thomasdezeeuw commented 4 months ago

@Thomasdezeeuw Is it ok for me to ping you for help?

Yes, sorry I don't have time to look at this today, maybe tomorrow.

Thomasdezeeuw commented 4 months ago

I think that the log method is not actually being called. I attempted to add various debug statements into the log method of the logger, and the debugging statements did not execute.

Could somebody please let me know if I am creating a logger instance incorrectly?

You need to call log::set_max_level(log::LevelFilter::Trace), by default all logs are filtered out.

DIvkov575 commented 4 months ago

@Thomasdezeeuw Thank you!

DIvkov575 commented 4 months ago

@Thomasdezeeuw

Do you know what might be causing MSRV to fail? Since the error is typically SetLoggerError, I think there might a preexisting logger blocking the 'setting' of a new one.

I was also wondering, how does the test build script work if it's just printing things? (It seems the necessary lib_build attribute is never passed to the tests, so when I had #[cfg_attr(lib_build, test)] my test was not recognized)

Thank You! Your answers mean a lot to me!

KodrAus commented 4 months ago

Hmm, that test infrastructure is all a bit tangled I think. I wonder if it would be easier to rename filters.rs to something like integration.rs and try combine this test into it? I think they are being treated as regular unit tests so can conflict trying to set the global logger.

DIvkov575 commented 4 months ago

I can try and combine them. However when combining them, should i just create a tuple (representing the state) or store the entire record (this might be problematic because Im pretty sure that not all of the sub-structs implement sync/send)?

KodrAus commented 4 months ago

I think defining a struct with just the state we want to test would cover it. It would be difficult to try capture the whole record, like you suggested it’s full of borrowed data and there’s no built-in way to create a shared version of one.

DIvkov575 commented 4 months ago

@KodrAus Not sure if my approach is wrong ... Im trying to creating a global state instance by using lazy_static and Arc

Should I change my approach in order to avoid external dependencies or should I use lazy_static/once_cell/std::sync::LazyStatic (stabilized only in 1.70)?

Also, will it be a problem if the tests will need to be executed sequentially?

KodrAus commented 4 months ago

@DIvkov575 It might be easier if we just roll the line number tests into exactly the main that we currently have. So the function test in there becomes test_filters, and we add a new one for the line number tests called test_line (or something along those lines) that we also call within that main where we've got an Arc<State> in scope. The change you've made to the State struct itself looks right. What do you think?

DIvkov575 commented 4 months ago

@KodrAus That will certainly simplify things. I just wasn't sure how important creating distinct/specific test was.

DIvkov575 commented 4 months ago

@KodrAus Just out of curiosity, what does the test harness do in Cargo.toml? I just saw the harnesses would be enabled/disabled but alternative harnesses were never passed as arguments.

Otherwise Is this good to merge?

KodrAus commented 3 months ago

Just out of curiosity, what does the test harness do in Cargo.toml?

This infrastructure here is all quite old, and I think it's mostly organized this way to test the max level Cargo features. I think setting harness = false makes cargo test run your binary as the test instead of looking for #[test] attributes in the file. I think it's mostly useful if you're plugging in a totally different tool to run your tests and I haven't really used it myself.

Nice job finding your way through this very non-standard piece of Rust!

Thomasdezeeuw commented 3 months ago

Thanks @DIvkov575

DIvkov575 commented 3 months ago

Just out of curiosity, what does the test harness do in Cargo.toml?

This infrastructure here is all quite old, and I think it's mostly organized this way to test the max level Cargo features. I think setting harness = false makes cargo test run your binary as the test instead of looking for #[test] attributes in the file. I think it's mostly useful if you're plugging in a totally different tool to run your tests and I haven't really used it myself.

Nice job finding your way through this very non-standard piece of Rust!

Thank you