test(loguru): Remove hardcoded line number in test_just_log #4552
Conversation
Use inspect to dynamically compute the correct line number instead of hard coding it. Fixes GH-4454
There was a problem hiding this comment.
ngl this is kinda cool, but it is really unclear to me how exactly this is supposed to work. I would want to see more code comments explaining how this logic works before going with the introspection solution.
I'm also unsure whether this fundamentally would solve the maintainability problem. Suppose some additional lines were added/removed between the inspect.currentframe() call and the call to the logger. Could the line number change in that case?
Honestly, I think it might be best to go with the simpler solution of writing a regex to ensure that the line number is present and valid, e.g. it matches \d+.
| @@ -54,12 +55,17 @@ def test_just_log( | |||
| ) | |||
| events = capture_events() | |||
|
|
|||
| frame = inspect.currentframe() | |||
| assert frame is not None | |||
| log_line_number = frame.f_lineno + 1 | |||
Replaced the inspection logic for the line number with a simple regex pattern that checks for the presence of an integer.
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #4552 +/- ##
==========================================
- Coverage 80.63% 80.63% -0.01%
==========================================
Files 156 156
Lines 16482 16482
Branches 2802 2802
==========================================
- Hits 13291 13290 -1
+ Misses 2308 2305 -3
- Partials 883 887 +4 |
| @@ -72,7 +73,8 @@ def test_just_log( | |||
| (breadcrumb,) = breadcrumbs | |||
| assert breadcrumb["level"] == expected_sentry_level | |||
| assert breadcrumb["category"] == "tests.integrations.loguru.test_loguru" | |||
| assert breadcrumb["message"][23:] == formatted_message | |||
| # Check that the message matches the expected pattern with a line number | |||
| assert re.search(expected_pattern, breadcrumb["message"][23:]) | |||
There was a problem hiding this comment.
I believe we want to use fullmatch rather than search. With search, the assertion passes if the string contains something matching the regex, but here, we instead want to ensure that the entire string matches
Make loguru test match against the pattern using fullmatch, so that the whole string is verified against the regex. Fixes GH-4454
Use inspect to dynamically compute the correct line number instead of hard coding it.
Fixes GH-4454
#4454
Changed the loguru test_just_log test to use inspection logic for the line number instead of hard coding it.