fix[tests]: clear log capture at start of local mode solver test

[yaugenst-flex]

This is a band-aid fix for the test coupling. Works for this specific case, but we'll need to do more robust warning capturing if we want to avoid dealing with this in the future.

[notion-workspace]

💑 Decouple tests

[caseyflex]

Actually, maybe we should wrap this in

with AssertLogLevel(log_capture, ...)

rather than the current context-free approach.

this might be the appropriate solution throughout?

I think the old assert_log_level syntax should be removed globally in favor of with AssertLogLevel(log_capture, "WARNING", contains_str="..."):

would this fix the threaded test issue?

[yaugenst-flex]

That's probably a good idea in general but does not fix the underlying issue here, since the context manager does not clear the captured logs. So essentially,

with AssertLogLevel(log_capture, "WARNING", contains_str="ModeSpec") as ctx:
    assert modes.n_group is None
    assert len(ctx.records) == 1

and

assert modes.n_group is None
assert len(log_capture) == 1

are equivalent in that sense. I changed it so we are not checking the length of the records at all and are instead only checking the actual message generated within the context manager, which should be ok for these tests?

I'll make an issue for doing this change globally but for now this will fix the failing CI.

[yaugenst-flex]

#2172

[caseyflex]

Hmmm, I would think it would fix the underlying issue since the context manager AssertLogLevel only checks records logged within its context. Is that incorrect?

[yaugenst-flex]

The way that AssertLogLevel is currently implemented it does not keep track of how many warnings were generated within its context, this is only calculated on __exit__ to determine which records to run assert_log_level against.

I think more fundamentally the issue is that many of our tests are written in a way that doesn't isolate individual warning cases, and some even work around the fact that there is some global logging state. I was intending this PR to be a quick fix to get CI passing so that it's not blocking other PRs anymore, but I guess I can come up with a more complete fix too.

[caseyflex]

Got it now regarding the context manager. Yeah up to you, this sounds good as a quick fix!

[yaugenst-flex]

Ok, bit the bullet. Main changes are:

• log_capture now cleans up after itself.
• AssertLogLevel now uses its own log handler (AssertLogLevelHandler) that it registers upon entry and deletes upon exit of the context. Within the context, only logs recorded by this handler will be checked. It also exposes a num_records property to make it easy to check the number of warnings raised within the context. Due to this, AssertLogLevel now no longer depends on an external log handler, so log_capture is not needed anymore. Good riddance, because AssertLogLevel used to rely on the fact that the records were passed by reference and manipulated by an outside source.
• Updated all tests to use AssertLogLevel where appropriate.

For the most part, the log_capture fixture is now not necessary anymore. The only use case for it is in conjunction with assert_log_level to check whether certain warnings (usually meaning no warnings) were raised up until a certain point in the test.

[caseyflex]

@yaugenst-flex this is awesome! should be much cleaner and safer this way.

One comment is that it really does seem like we can just get rid of log_capture altogether now. assert_log_level is only used in a couple places, where it isn't really needed -- I put it there just to ensure that the warning was coming from the correct call, before we had AssertLogLevel. If we wanted to check warnings up to a certain point, we could just wrap the whole block in AssertLogLevel. And keeping log_capture around seems more likely to cause these issues to crop up again in the future.

[yaugenst-flex]

Yeah good point, I was considering this but was a bit hesitant to change the behavior of the tests. Updated, log_capture is gone :)

[daquinteroflex]

Nice idea on the raise with AssertLogLevel! Looks good, thanks Yannick.

[notion-workspace]

⏫ Remove legacy assert_log_level syntax in tests