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

add string matching to assert_log_level #1327

Merged
merged 1 commit into from
Dec 20, 2023
Merged

Conversation

tylerflex
Copy link
Collaborator

No description provided.

@momchil-flex
Copy link
Collaborator

I think what would also be good to have is a context manager that avoids us doing log_capture.clear() explicitly. Something like:

with AssertLogLeveL(log_capture, log_level_expected, capture_str):
    do_something()

and just use this everywhere instead of the current assert_log_level. What do you think?

@tylerflex
Copy link
Collaborator Author

I think that could be nice, yea. I can work on that and add a commit to this branch.

Copy link
Collaborator Author

@tylerflex tylerflex left a comment

Choose a reason for hiding this comment

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

@momchil-flex I added the context manager, I do think this improves the workflow a lot

with AssertLogLevel(log_capture, "WARNING"):
    # do stuff

also updated all tests to use it

@momchil-flex
Copy link
Collaborator

That's nice! Only thing I'm wondering about is that the context manager affects the passed results. In principle it may be better practice to not clear but just to only examine the results that are added during the execution, e.g. if we have a list of outputs, save the length upon entering, and then iterate from that length to the new list length when exiting (also not clearing there). In practice, this is good for our purposes though. But for example, if you don't clear the log, and you run the pytest with -rA, at the end it will show you everything that was captured along the way in stderr output. Now, it will show empty. What do you think?

@tylerflex
Copy link
Collaborator Author

tylerflex commented Dec 20, 2023

yea I agree, this is probably better. I added a commit that only clears a deepcopy() of the original log_capture. that way the original is not affected.

This made a couple tests fail in test_medium.py that I'm coordinating with @weiliangjin2021 about. Basically these two (original) lines

    # anisotropic medium, warn allow_gain is ignored
    _ = td.AnisotropicMedium(xx=td.Medium(), yy=mL, zz=mS, allow_gain=True)
    assert_log_level(log_capture, "WARNING")

    _ = td.AnisotropicMedium(xx=td.Medium(), yy=mL, zz=mS, allow_gain=False)
    assert_log_level(log_capture, "WARNING")

I think this is the validator in question.

perhaps were just capturing the warnings from the previous lines. I'm not sure if there's a bug in AnisotropicMedium or we dont expect any warnings since allow_gain is specified and the log level should be None.

@tylerflex
Copy link
Collaborator Author

note @momchil-flex I just force pushed to resolve conflicts with develop.

@tylerflex tylerflex force-pushed the tyler/test/contains_str branch 2 times, most recently from 7b17a31 to edd01b8 Compare December 20, 2023 15:31
@tylerflex
Copy link
Collaborator Author

Ok actually this uncovered a bug in my own implementation. I adjusted the logic and it should work now. Basically how it works is that the context manager never clears the log_capture, but it only looks at the records introduced in the context (between __enter__ and __exit__).

@momchil-flex
Copy link
Collaborator

Great. Seems like deepcopy is unused now. We can merge afterwards.

@tylerflex tylerflex merged commit c1addaa into develop Dec 20, 2023
14 checks passed
@tylerflex tylerflex deleted the tyler/test/contains_str branch December 20, 2023 19:50
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.

2 participants