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

adding line_number test + updating some testing infrastructure #619

Merged
merged 10 commits into from
Mar 26, 2024

Conversation

DIvkov575
Copy link
Contributor

@DIvkov575 DIvkov575 commented Mar 16, 2024

resolves #601

  • adding line_number test into new file integration.rs
  • moving tests from filter.rs -> integration.rs
  • updating logger from filter.rs in order to support both filter and line_number tests

@DIvkov575
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@Thomasdezeeuw
Copy link
Collaborator

@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
Copy link
Collaborator

Thomasdezeeuw commented Mar 19, 2024

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
Copy link
Contributor Author

@Thomasdezeeuw Thank you!

@DIvkov575
Copy link
Contributor Author

@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
Copy link
Contributor

KodrAus commented Mar 19, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

KodrAus commented Mar 20, 2024

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 DIvkov575 changed the title Add line number tests adding line_number test + updating some testing infrastructure Mar 23, 2024
@DIvkov575
Copy link
Contributor Author

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

  • a global AtomicBool to determine whether the setup function (containing setlogger) has been executed

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
Copy link
Contributor

KodrAus commented Mar 24, 2024

@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
Copy link
Contributor Author

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

@DIvkov575
Copy link
Contributor Author

@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?

Copy link
Contributor

@KodrAus KodrAus left a comment

Choose a reason for hiding this comment

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

Thanks for working through this @DIvkov575! This looks good to me, and should help future efforts to improve our test coverage for this type of thing.

@KodrAus
Copy link
Contributor

KodrAus commented Mar 26, 2024

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!

@KodrAus KodrAus merged commit 4961162 into rust-lang:master Mar 26, 2024
14 checks passed
@Thomasdezeeuw
Copy link
Collaborator

Thanks @DIvkov575

@DIvkov575
Copy link
Contributor Author

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

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.

Add tests that check the line numbers
3 participants