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

Log the list of tests in a given batch #21549

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jasondamour
Copy link

@jasondamour jasondamour commented Oct 17, 2024

This PR will log the batched test files before tests execute:

09:59:41.68 [INFO] batch of a_test.py:tests and 3 other files contains:
  a_test.py:tests
  b_test.py:tests
  c_test.py:tests
  d_test.py:tests

I would like some input on the configuration for this option. Personally, I would like this to be info in CI so we can always know that tests are in CI, but maybe debug locally. Is there any existing convention on variable log levels, like

# pants.toml
[test]
batch_log_level = "info" | "debug" ...?

I see log_levels_by_target, is there a "target" (confusing overloading of that word) for the batch system?

@jasondamour jasondamour marked this pull request as ready for review October 22, 2024 16:31
@benjyw
Copy link
Contributor

benjyw commented Oct 24, 2024

Nice! I suppose this should be debug by default but turned to info specifically in CI config.

You can check for the relevant logging target by setting --show-log-target, which will then show the names in the log lines.

@cognifloyd
Copy link
Member

cognifloyd commented Oct 25, 2024

I think a configurable log level in pants.toml is the way to go. Your naming of [test].batch_log_level is fine.

I wonder if people will want to start tuning the level of other messages. You could make it extensible by adding something like [test.log_levels_by_message] (though that might be over engineering at this point):

[test.experimental_log_levels_by_message]
files_in_batch = "info"

@benjyw
Copy link
Contributor

benjyw commented Oct 27, 2024

I think a configurable log level in pants.toml is the way to go. Your naming of [test].batch_log_level is fine.

I am in friendly disagreement. Why single this feature out for special log configuration when we already have a robust global mechanism for adjusting the log level on a component-by-component basis?

@jasondamour
Copy link
Author

I did a quick test with --show-log-target, and unfortunately it seems like all of the "test process" logic is part of workunit_store log target, which isn't the level of granularity i was hoping for to single out this log event

@benjyw
Copy link
Contributor

benjyw commented Oct 29, 2024

I did a quick test with --show-log-target, and unfortunately it seems like all of the "test process" logic is part of workunit_store log target, which isn't the level of granularity i was hoping for to single out this log event

Can we not introduce a new log target though? I haven't spelunked the code to see how to do that, but I have to hope it's straightforward...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants