track example log fixtures so CLI tests find them on a fresh clone - #25
Open
HrachShah wants to merge 10 commits into
Open
track example log fixtures so CLI tests find them on a fresh clone#25HrachShah wants to merge 10 commits into
HrachShah wants to merge 10 commits into
Conversation
added 3 commits
June 26, 2026 08:09
…ree on tz-awareness In utils.filter_lines and cli._parse_file, when a parsed log timestamp carries tzinfo and the CLI --start-time/--end-time filter was parsed as a naive datetime, Python raises "can't compare offset-naive and offset-aware datetimes" at the timestamp < start_time check and the whole analyze command crashes with a traceback. The reverse case (naive parsed timestamp against an aware filter) hits the same crash. Add a small helper that normalises the parsed timestamp into the same tz-awareness as the filter boundaries -- strip tzinfo when either boundary is naive, attach UTC when either boundary is aware -- and call it right after parse_timestamp so the < and > comparisons succeed. The original parsed timestamp object on the line is not modified; only the locally-copied value used for comparison is normalised, so the rest of the pipeline (analyzer, time distribution) sees the original.
The previous commit fixes the offset-naive vs offset-aware comparison crash by introducing _align_datetime_to_filter and calling it from both utils.filter_lines and cli._parse_file. Add unit tests that exercise both helpers directly (six cases covering None, naive/aware input against naive/aware filters, and the end-only filter stripping aware tzinfo) and end-to-end integration cases through filter_lines (three cases: aware start vs naive log, naive start vs aware log, and a boundary check that confirms entries before the start are filtered out without crashing). All nine tests fail against the pre-fix tree and pass with the fix.
The four fixtures under examples/ (syslog-sample, app-json, apache-sample, mixed) are the canonical inputs to tests/conftest.py:examples_dir() and the matching syslog_file()/json_file()/apache_file()/mixed_file() fixtures, which the CLI tests pass directly to `runner.invoke(main, ['analyze', ...])`. When any of them is missing, Click's file_okay=True path validation rejects the path with BadParameter 'File ... does not exist.' and Click exits 2, so the test fails on the very first line (`assert result.exit_code == 0`) and any parser-level bug downstream is masked. The reason they keep disappearing: `examples/*.log` is re-ignored at the bottom of .gitignore, but the four `!examples/<fixture>.log` exceptions sitting ABOVE that line never win — gitignore evaluates 'last matching pattern decides', so the re-ignore overrides every earlier negation. Move the generic `examples/*.log` re-ignore to the top of the examples block and put the four `!examples/<fixture>.log` negations AFTER it. That ordering lets the negations override the re-ignore for the four tracked fixtures while still keeping ad-hoc *.log files in that folder ignored (so people don't accidentally commit scratch logs). Verified: 56 tests pass on a clean tree (tests/test_cli.py::TestCLI's test_analyze_syslog_file, test_analyze_json_file, test_analyze_apache_file, test_analyze_output_json all resolve their fixture path now).
Reviewer's GuideThis PR fixes datetime timezone mismatches in CLI log filtering and ensures example log fixtures are tracked so CLI tests run correctly on a fresh clone, by adding a helper to align parsed timestamps with CLI time filters, wiring it into both filtering paths, adding tests, and reordering .gitignore entries while checking in the example logs themselves. Sequence diagram for CLI log filtering with timezone alignmentsequenceDiagram
actor CLIUser
participant main
participant filter_lines
participant _parse_file
participant parse_timestamp
participant _align_datetime_to_filter
CLIUser->>main: analyze --start-time/--end-time
main->>filter_lines: filter_lines(path, start_time, end_time)
loop for each line in file
filter_lines->>parse_timestamp: parse_timestamp(line)
parse_timestamp-->>filter_lines: timestamp
filter_lines->>_align_datetime_to_filter: _align_datetime_to_filter(timestamp, start_time, end_time)
_align_datetime_to_filter-->>filter_lines: aligned_timestamp
filter_lines->>filter_lines: [compare aligned_timestamp to start_time/end_time]
end
main->>_parse_file: _parse_file(path, start_time, end_time, search_pattern)
loop for each matched line
_parse_file->>parse_timestamp: parse_timestamp(line)
parse_timestamp-->>_parse_file: timestamp
_parse_file->>_align_datetime_to_filter: _align_datetime_to_filter(timestamp, start_time, end_time)
_align_datetime_to_filter-->>_parse_file: aligned_timestamp
_parse_file->>_parse_file: [compare aligned_timestamp to start_time/end_time]
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds timezone normalization for log parsing and filtering, resets analyzer error-group state between runs, improves parser normalization, routes CLI format sampling through shared log reading, adds regression coverage, and explicitly tracks selected example log fixtures. ChangesLog analysis behavior updates
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Since
_align_datetime_to_filteris now a cross-cutting utility used by bothfilter_linesand the CLI, consider making it a public helper (or documenting its intended use) rather than importing a private function fromutilsintocli.py. - The alignment logic in
_align_datetime_to_filteris subtle; it may be helpful to split the naive→aware and aware→naive cases into clearly named branches or helper functions to make the intended precedence and behavior easier to follow for future changes. - Given that
_parse_fileduplicates parts offilter_lines(including timestamp parsing and filtering), consider whether it could reusefilter_linesdirectly to avoid divergence in future changes to the timestamp handling logic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since `_align_datetime_to_filter` is now a cross-cutting utility used by both `filter_lines` and the CLI, consider making it a public helper (or documenting its intended use) rather than importing a private function from `utils` into `cli.py`.
- The alignment logic in `_align_datetime_to_filter` is subtle; it may be helpful to split the naive→aware and aware→naive cases into clearly named branches or helper functions to make the intended precedence and behavior easier to follow for future changes.
- Given that `_parse_file` duplicates parts of `filter_lines` (including timestamp parsing and filtering), consider whether it could reuse `filter_lines` directly to avoid divergence in future changes to the timestamp handling logic.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/log_analyzer_cli/cli.py (1)
198-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicate helper from
cli.py.
_parse_file()now imports_align_datetime_to_filterfromlog_analyzer_cli.utils, butcli.pystill has a same-named top-level implementation at Lines 12-35. Keeping both copies makes future fixes easy to miss. Import the shared helper once at module scope and delete the local copy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/log_analyzer_cli/cli.py` at line 198, `cli.py` still contains a local `_align_datetime_to_filter` implementation even though `_parse_file()` now imports the shared helper from `log_analyzer_cli.utils`, so remove the duplicate top-level copy and keep only the module-scope import. Update the `cli.py` module to use the shared `_align_datetime_to_filter` everywhere, and ensure no references still point to the deleted local helper so future changes stay centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_utils.py`:
- Line 87: The test file is missing a trailing newline, which is triggering
flake8 W292. Update the end of tests/test_utils.py so the final line in the test
helper section is properly terminated with a newline, keeping the existing
assertion in place and ensuring the file ends cleanly.
---
Nitpick comments:
In `@src/log_analyzer_cli/cli.py`:
- Line 198: `cli.py` still contains a local `_align_datetime_to_filter`
implementation even though `_parse_file()` now imports the shared helper from
`log_analyzer_cli.utils`, so remove the duplicate top-level copy and keep only
the module-scope import. Update the `cli.py` module to use the shared
`_align_datetime_to_filter` everywhere, and ensure no references still point to
the deleted local helper so future changes stay centralized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4dbb2229-6add-4405-bf3b-66d98d8f9031
⛔ Files ignored due to path filters (4)
examples/apache-sample.logis excluded by!**/*.logexamples/app-json.logis excluded by!**/*.logexamples/mixed.logis excluded by!**/*.logexamples/syslog-sample.logis excluded by!**/*.log
📒 Files selected for processing (4)
.gitignoresrc/log_analyzer_cli/cli.pysrc/log_analyzer_cli/utils.pytests/test_utils.py
added 3 commits
July 8, 2026 19:53
…nalyzers don't merge groups LogAnalyzer.analyze() previously kept self._error_patterns across calls, so a second analyze() on the same instance that contained the same error message as a previous call reported count == 2 even though only one matching entry was in the second batch. The internal accumulator is implementation state, not a cumulative tally, so each analyze() starts with a fresh dict. Add test_repeated_analyze_does_not_carry_over_error_groups covering the single-instance double-call case: pre-fix code gave count == 2 for the second batch's single matching entry; post-fix gives count == 1 as the result of analyzing just that one entry.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_parsers.py (1)
84-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the numeric-seconds branch as well.
This test exercises only the millisecond path. Add a seconds-based timestamp case so both changed conversion branches are protected against timezone regressions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_parsers.py` around lines 84 - 85, Add a seconds-based timestamp case to the relevant parser test alongside the existing millisecond case, and assert its parsed timestamp has non-null tzinfo and a zero UTC offset. Keep the current millisecond coverage unchanged so both numeric-seconds and numeric-milliseconds conversion branches are validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_parsers.py`:
- Around line 84-85: Add a seconds-based timestamp case to the relevant parser
test alongside the existing millisecond case, and assert its parsed timestamp
has non-null tzinfo and a zero UTC offset. Keep the current millisecond coverage
unchanged so both numeric-seconds and numeric-milliseconds conversion branches
are validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a45eb6a-96f6-4a17-9de4-ec6f72efe8f0
📒 Files selected for processing (2)
src/log_analyzer_cli/parsers/json_log.pytests/test_parsers.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_cli.py (1)
87-96: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that auto-detection selected the JSON parser.
The test currently passes if the CLI falls back to
GenericParser; assert the parser announcement as well so this regression test verifies the intended compressed JSON detection path.assert result.exit_code == 0 + assert "Using parser: json" in result.output assert "LOG ANALYSIS REPORT" in result.output🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cli.py` around lines 87 - 96, Update test_analyze_auto_format_detection_for_gzip to assert that the CLI output announces selection of the JSON parser, in addition to the existing successful exit and report assertions, so the test rejects GenericParser fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/log_analyzer_cli/cli.py`:
- Around line 163-168: Update the sample collection loop in the CLI log-analysis
flow to count only non-empty, stripped lines toward the limit of 10. Continue
scanning past blank physical lines, append each valid sample, and stop once
sample_lines contains 10 entries rather than based on enumerate’s physical-line
index.
---
Nitpick comments:
In `@tests/test_cli.py`:
- Around line 87-96: Update test_analyze_auto_format_detection_for_gzip to
assert that the CLI output announces selection of the JSON parser, in addition
to the existing successful exit and report assertions, so the test rejects
GenericParser fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed6866b8-5a98-4e2a-a20b-c91f15dd3b4b
📒 Files selected for processing (2)
src/log_analyzer_cli/cli.pytests/test_cli.py
Comment on lines
+163
to
+168
| for i, line in enumerate(read_log_file(file_path)): | ||
| if i >= 10: | ||
| break | ||
| line = line.strip() | ||
| if line: | ||
| sample_lines.append(line) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Collect 10 non-empty samples, not 10 physical lines.
i >= 10 stops scanning after the first 10 physical lines, so logs with leading blank lines may produce an empty sample and incorrectly return no parser even when valid entries follow.
Proposed fix
- for i, line in enumerate(read_log_file(file_path)):
- if i >= 10:
- break
+ for line in read_log_file(file_path):
line = line.strip()
if line:
sample_lines.append(line)
+ if len(sample_lines) >= 10:
+ break📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for i, line in enumerate(read_log_file(file_path)): | |
| if i >= 10: | |
| break | |
| line = line.strip() | |
| if line: | |
| sample_lines.append(line) | |
| for line in read_log_file(file_path): | |
| line = line.strip() | |
| if line: | |
| sample_lines.append(line) | |
| if len(sample_lines) >= 10: | |
| break |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/log_analyzer_cli/cli.py` around lines 163 - 168, Update the sample
collection loop in the CLI log-analysis flow to count only non-empty, stripped
lines toward the limit of 10. Continue scanning past blank physical lines,
append each valid sample, and stop once sample_lines contains 10 entries rather
than based on enumerate’s physical-line index.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/log_analyzer_cli/utils.py (1)
7-37: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the instant when converting aware timestamps to naive filters.
The aware-to-naive path strips
tzinfowithout first converting to UTC. Sincesrc/log_analyzer_cli/parsers/apache.pyaccepts arbitrary%zoffsets,10:00+05:00becomes naive10:00instead of05:00, which can silently include or exclude entries. Convert aware timestamps to the agreed reference timezone before removingtzinfo, and add a non-UTC regression test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/log_analyzer_cli/utils.py` around lines 7 - 37, Update _align_datetime_to_filter so aware timestamps are converted to the agreed UTC reference timezone before stripping tzinfo when matching naive filter boundaries. Preserve the existing behavior for already-naive timestamps and fully aware comparisons, and add a regression test using a non-UTC offset such as +05:00 to verify the instant remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/log_analyzer_cli/utils.py`:
- Around line 138-144: Update the hexadecimal substitution in the
pattern-normalization logic to match both lowercase and uppercase prefixes,
while preserving the existing full-token <HEX> replacement before the generic
numeric pass. Add coverage verifying equivalent handling for 0x and 0X inputs.
---
Outside diff comments:
In `@src/log_analyzer_cli/utils.py`:
- Around line 7-37: Update _align_datetime_to_filter so aware timestamps are
converted to the agreed UTC reference timezone before stripping tzinfo when
matching naive filter boundaries. Preserve the existing behavior for
already-naive timestamps and fully aware comparisons, and add a regression test
using a non-UTC offset such as +05:00 to verify the instant remains unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a94f1d96-0e4f-4ab7-a511-b77bccb1f84f
📒 Files selected for processing (4)
src/log_analyzer_cli/parsers/apache.pysrc/log_analyzer_cli/utils.pytests/test_parsers.pytests/test_utils.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_parsers.py
- tests/test_utils.py
Comment on lines
+138
to
144
| # Replace hex values before the generic number pass so 0x values stay | ||
| # grouped as one placeholder. | ||
| pattern = re.sub(r'0x[0-9a-fA-F]+', '<HEX>', pattern) | ||
|
|
||
| # Replace remaining standalone numbers | ||
| pattern = re.sub(r'\b\d+\b', '<NUM>', pattern) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle uppercase hexadecimal prefixes.
The 0x... pattern does not match 0X...; the later numeric pass then produces output such as <NUM>XDEADBEEF instead of <HEX>. Use a case-insensitive pattern and add coverage for both prefixes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/log_analyzer_cli/utils.py` around lines 138 - 144, Update the hexadecimal
substitution in the pattern-normalization logic to match both lowercase and
uppercase prefixes, while preserving the existing full-token <HEX> replacement
before the generic numeric pass. Add coverage verifying equivalent handling for
0x and 0X inputs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Four fixtures under
examples/(syslog-sample.log,app-json.log,apache-sample.log,mixed.log) are the inputs totests/conftest.py'ssyslog_file()/json_file()/apache_file()/mixed_file()fixtures. The CLI tests callrunner.invoke(main, ['analyze', str(<fixture>)]), and when any fixture is missing Click'sfile_okay=Truevalidation rejects the path withBadParameterand Click exits 2, so the test fails onassert result.exit_code == 0and any parser-level bug downstream is masked.Why they keep vanishing
The bottom of
.gitignorere-ignoresexamples/*.log, and the four!examples/<fixture>.lognegations sit above that line. Gitignore evaluates 'last matching pattern decides', so the re-ignore overrides every earlier negation and the fixtures never get tracked.Fix
Reorder the examples block in
.gitignore: put the genericexamples/*.logre-ignore first, then the four!examples/<fixture>.lognegations after it. That ordering lets the negations override the re-ignore for the four tracked fixtures while still keeping ad-hoc*.logfiles in that folder ignored.Verified:
pytest -p no:httpbinreports 56/56 passing on a clean tree, including the previously-failing CLI tests.Summary by Sourcery
Normalize log timestamp comparisons against CLI time filters and ensure example log fixtures are tracked so CLI tests pass on fresh clones.
New Features:
Bug Fixes:
Build:
Tests:
Summary by CodeRabbit
<HEX>.analyze()behavior, and<HEX>normalization..gitignoreto explicitly track specific example fixture log files while continuing to ignore otherexamples/*.logfiles.