test: add OTLP output support to integration tests#1012
Open
Molter73 wants to merge 1 commit into
Open
Conversation
|
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:
📝 WalkthroughWalkthroughThe container build accepts configurable Cargo arguments, including the OTEL feature. Make targets expose OTEL and OTLP test modes. Test infrastructure adds shared gRPC and OTLP event servers, protocol-independent comparisons, output-mode parametrization, and updates existing tests to use the shared server abstraction. ChangesEvent output testing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant FactContainer
participant GrpcServer
participant OtlpServer
participant EventServer
FactContainer->>GrpcServer: send gRPC FileActivity events
FactContainer->>OtlpServer: POST /v1/logs
GrpcServer->>EventServer: enqueue translated Event
OtlpServer->>EventServer: enqueue translated Event
EventServer->>FactContainer: compare wait_events expectations
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1012 +/- ##
=======================================
Coverage 32.95% 32.95%
=======================================
Files 21 21
Lines 2971 2971
Branches 2971 2971
=======================================
Hits 979 979
Misses 1989 1989
Partials 3 3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Molter73
force-pushed
the
mauro/feat/otel-output
branch
from
a52c44e to
b915ce8
Compare
Molter73
force-pushed
the
mauro/tests/otel-support
branch
from
85722a4 to
e2a1695
Compare
Molter73
force-pushed
the
mauro/feat/otel-output
branch
2 times, most recently
from
4214811 to
3629e66
Compare
Molter73
force-pushed
the
mauro/tests/otel-support
branch
2 times, most recently
from
f964087 to
5c66700
Compare
Molter73
force-pushed
the
mauro/tests/otel-support
branch
from
5c66700 to
69acc30
Compare
![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 (2)
Makefile (1)
23-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider deduplicating
imageandimage-oteltargets.
image-otel(lines 23-30) repeats the entireimagerecipe (lines 15-21) with only an extra--build-argand a different tag. A shared variable/pattern rule would remove the duplication and also resolves the checkmake body-length warning below.♻️ Suggested refactor
-image: - $(DOCKER) build \ - -f Containerfile \ - --build-arg FACT_VERSION=$(FACT_VERSION) \ - --build-arg RUST_VERSION=$(RUST_VERSION) \ - -t $(FACT_IMAGE_NAME) \ - $(CURDIR) - -image-otel: - $(DOCKER) build \ - -f Containerfile \ - --build-arg FACT_VERSION=$(FACT_VERSION) \ - --build-arg RUST_VERSION=$(RUST_VERSION) \ - --build-arg CARGO_ARGS="--features otel" \ - -t $(FACT_IMAGE_NAME)-otel \ - $(CURDIR) +DOCKER_BUILD = $(DOCKER) build \ + -f Containerfile \ + --build-arg FACT_VERSION=$(FACT_VERSION) \ + --build-arg RUST_VERSION=$(RUST_VERSION) + +image: + $(DOCKER_BUILD) -t $(FACT_IMAGE_NAME) $(CURDIR) + +image-otel: + $(DOCKER_BUILD) --build-arg CARGO_ARGS="--features otel" -t $(FACT_IMAGE_NAME)-otel $(CURDIR)Static analysis (checkmake) flags
image-otelfor exceeding the max body length (7 lines vs 5 allowed).🤖 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 `@Makefile` around lines 23 - 31, Deduplicate the Docker build recipes for the image and image-otel targets. Refactor the shared arguments and build command into a reusable variable or pattern rule, while allowing image-otel to add the otel CARGO_ARGS value and use its distinct tag; update both targets to invoke the shared logic and keep image-otel within the checkmake body-length limit.Source: Linters/SAST tools
tests/server.py (1)
380-389: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove leftover debug prints in ACL translation.
print('#### Got ACL event')andprint(f'{kwargs}')are debug artifacts that will spam test output on every ACL event processed via OTLP.🧹 Proposed fix
elif event_type == EventType.ACL: - print('#### Got ACL event') kwargs['acl_type'] = ACL_TYPE_MAP.get(file_data.get('acl_type')) kwargs['acl_entries'] = [ OtlpServer._acl_entry_translate(entry) for entry in file_data.get('entries', []) ] - - print(f'{kwargs}')🤖 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/server.py` around lines 380 - 389, Remove the two leftover debug print statements from the EventType.ACL branch that translates ACL events, while preserving the acl_type and acl_entries assignments and translation behavior.
🤖 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/utils.py`:
- Around line 61-68: Update the single-quote fallback in the string-quoting
helper to use POSIX escaping: replace each literal apostrophe with the `'\''`
sequence rather than `\'`. Preserve the existing double-quote path and ensure
inputs containing apostrophes, including those with `$` or backticks, produce
valid shell arguments.
---
Nitpick comments:
In `@Makefile`:
- Around line 23-31: Deduplicate the Docker build recipes for the image and
image-otel targets. Refactor the shared arguments and build command into a
reusable variable or pattern rule, while allowing image-otel to add the otel
CARGO_ARGS value and use its distinct tag; update both targets to invoke the
shared logic and keep image-otel within the checkmake body-length limit.
In `@tests/server.py`:
- Around line 380-389: Remove the two leftover debug print statements from the
EventType.ACL branch that translates ACL events, while preserving the acl_type
and acl_entries assignments and translation behavior.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: bdfa6f98-bfb0-45ea-ba86-4ae054632132
📒 Files selected for processing (25)
ContainerfileMakefiletests/Makefiletests/conftest.pytests/event.pytests/requirements.txttests/server.pytests/test_acl.pytests/test_config_hotreload.pytests/test_editors/test_nvim.pytests/test_editors/test_sed.pytests/test_editors/test_vi.pytests/test_editors/test_vim.pytests/test_file_open.pytests/test_misc.pytests/test_path_chmod.pytests/test_path_chown.pytests/test_path_mkdir.pytests/test_path_rename.pytests/test_path_rmdir.pytests/test_path_unlink.pytests/test_rate_limit.pytests/test_wildcard.pytests/test_xattr.pytests/utils.py
Collaborator
Author
|
Apparently I must have dropped some change while rebasing (which is honestly weird since all PRs in this stacked set are a single commit each), the tests are failing in a way I swear I've fixed already. |
Molter73
force-pushed
the
mauro/tests/otel-support
branch
from
69acc30 to
046a472
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/server.py (1)
380-388: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove leftover debug prints.
print('#### Got ACL event')andprint(f'{kwargs}')look like debugging artifacts and add noise to test output.♻️ Proposed cleanup
elif event_type == EventType.ACL: - print('#### Got ACL event') kwargs['acl_type'] = ACL_TYPE_MAP.get(file_data.get('acl_type')) kwargs['acl_entries'] = [ OtlpServer._acl_entry_translate(entry) for entry in file_data.get('entries', []) ] - - print(f'{kwargs}')🤖 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/server.py` around lines 380 - 388, Remove the leftover debug print statements from the EventType.ACL branch, specifically the `print('#### Got ACL event')` and `print(f'{kwargs}')` calls, while preserving the ACL translation and kwargs construction logic.
🤖 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/server.py`:
- Around line 380-388: Remove the leftover debug print statements from the
EventType.ACL branch, specifically the `print('#### Got ACL event')` and
`print(f'{kwargs}')` calls, while preserving the ACL translation and kwargs
construction logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: fbdae493-138b-4d46-b687-5c4451ecadb0
📒 Files selected for processing (25)
ContainerfileMakefiletests/Makefiletests/conftest.pytests/event.pytests/requirements.txttests/server.pytests/test_acl.pytests/test_config_hotreload.pytests/test_editors/test_nvim.pytests/test_editors/test_sed.pytests/test_editors/test_vi.pytests/test_editors/test_vim.pytests/test_file_open.pytests/test_misc.pytests/test_path_chmod.pytests/test_path_chown.pytests/test_path_mkdir.pytests/test_path_rename.pytests/test_path_rmdir.pytests/test_path_unlink.pytests/test_rate_limit.pytests/test_wildcard.pytests/test_xattr.pytests/utils.py
✅ Files skipped from review due to trivial changes (8)
- tests/test_path_mkdir.py
- tests/test_misc.py
- tests/test_file_open.py
- tests/test_path_rmdir.py
- tests/test_path_chmod.py
- tests/test_acl.py
- tests/test_path_chown.py
- tests/test_xattr.py
🚧 Files skipped from review as they are similar to previous changes (15)
- tests/Makefile
- tests/requirements.txt
- Containerfile
- tests/test_rate_limit.py
- tests/test_editors/test_vim.py
- tests/test_editors/test_vi.py
- tests/test_editors/test_nvim.py
- tests/test_editors/test_sed.py
- tests/utils.py
- tests/test_path_unlink.py
- tests/test_path_rename.py
- tests/test_wildcard.py
- tests/test_config_hotreload.py
- tests/conftest.py
- tests/event.py
Molter73
force-pushed
the
mauro/tests/otel-support
branch
from
046a472 to
693ce75
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/server.py (1)
382-390: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove leftover debug prints.
The
#### Got ACL eventandf'{kwargs}'prints look like debugging artifacts and will spam test output.♻️ Proposed cleanup
elif event_type == EventType.ACL: - print('#### Got ACL event') kwargs['acl_type'] = ACL_TYPE_MAP.get(file_data.get('acl_type')) kwargs['acl_entries'] = [ OtlpServer._acl_entry_translate(entry) for entry in file_data.get('entries', []) ] - - print(f'{kwargs}')🤖 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/server.py` around lines 382 - 390, Remove the two leftover debug print statements from the EventType.ACL branch, including the “Got ACL event” message and the kwargs dump, while preserving the ACL translation and assignment logic.
🤖 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/server.py`:
- Around line 382-390: Remove the two leftover debug print statements from the
EventType.ACL branch, including the “Got ACL event” message and the kwargs dump,
while preserving the ACL translation and assignment logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: cf9a8bba-0625-4396-af77-514bb84a348a
📒 Files selected for processing (25)
ContainerfileMakefiletests/Makefiletests/conftest.pytests/event.pytests/requirements.txttests/server.pytests/test_acl.pytests/test_config_hotreload.pytests/test_editors/test_nvim.pytests/test_editors/test_sed.pytests/test_editors/test_vi.pytests/test_editors/test_vim.pytests/test_file_open.pytests/test_misc.pytests/test_path_chmod.pytests/test_path_chown.pytests/test_path_mkdir.pytests/test_path_rename.pytests/test_path_rmdir.pytests/test_path_unlink.pytests/test_rate_limit.pytests/test_wildcard.pytests/test_xattr.pytests/utils.py
✅ Files skipped from review due to trivial changes (6)
- tests/test_rate_limit.py
- tests/test_path_rmdir.py
- tests/test_path_unlink.py
- tests/test_misc.py
- tests/test_path_rename.py
- tests/test_path_chmod.py
🚧 Files skipped from review as they are similar to previous changes (17)
- tests/requirements.txt
- Containerfile
- tests/Makefile
- tests/test_editors/test_nvim.py
- tests/test_editors/test_vi.py
- tests/test_path_mkdir.py
- tests/test_editors/test_sed.py
- tests/utils.py
- tests/test_editors/test_vim.py
- tests/test_path_chown.py
- tests/test_file_open.py
- tests/test_config_hotreload.py
- tests/test_acl.py
- tests/test_wildcard.py
- tests/event.py
- tests/conftest.py
- tests/test_xattr.py
Molter73
force-pushed
the
mauro/tests/otel-support
branch
2 times, most recently
from
735c159 to
16f5d71
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/server.py (1)
112-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale parameter name after refactor.
skip_oneof_namesused to hold protobuf oneof field-name strings; it now holdsEventTypeenum members (per the type hint and call site). The name is misleading for future readers.🤖 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/server.py` around lines 112 - 118, The _wait_events method uses the misleading parameter name skip_oneof_names even though it now contains EventType enum members. Rename this parameter to reflect its EventType-based contents and update all references and call sites consistently.Makefile (1)
15-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared build args to avoid duplicating the
imagetarget.
image-otelrepeats the entire body ofimageverbatim aside from one extra--build-argand the tag. This drives the checkmakemaxbodylengthwarning and creates a maintenance hazard (a future change to shared args must be applied in two places).♻️ Proposed refactor
+COMMON_BUILD_ARGS := \ + -f Containerfile \ + --build-arg FACT_VERSION=$(FACT_VERSION) \ + --build-arg RUST_VERSION=$(RUST_VERSION) + image: $(DOCKER) build \ - -f Containerfile \ - --build-arg FACT_VERSION=$(FACT_VERSION) \ - --build-arg RUST_VERSION=$(RUST_VERSION) \ + $(COMMON_BUILD_ARGS) \ -t $(FACT_IMAGE_NAME) \ $(CURDIR) image-otel: $(DOCKER) build \ - -f Containerfile \ - --build-arg FACT_VERSION=$(FACT_VERSION) \ - --build-arg RUST_VERSION=$(RUST_VERSION) \ + $(COMMON_BUILD_ARGS) \ --build-arg CARGO_ARGS="--features otel" \ -t $(FACT_IMAGE_NAME)-otel \ $(CURDIR)🤖 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 `@Makefile` around lines 15 - 31, Extract the shared Docker build arguments and commands from the image and image-otel targets into reusable Make variables or a common target, then have each target supply only its distinct image tag and optional CARGO_ARGS value. Update image and image-otel to use the shared implementation so common build arguments are defined in one place.Source: Linters/SAST tools
🤖 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/server.py`:
- Around line 149-163: Update the skip_oneof_names construction in the relevant
event-search method to replace EVENT_TYPE_MAP.get(key, EventType.UNKNOWN) with
EVENT_TYPE_MAP[key], while preserving SKIP_EVENT_TYPES precedence, so
unrecognized skip keys raise KeyError instead of silently becoming a no-op.
---
Nitpick comments:
In `@Makefile`:
- Around line 15-31: Extract the shared Docker build arguments and commands from
the image and image-otel targets into reusable Make variables or a common
target, then have each target supply only its distinct image tag and optional
CARGO_ARGS value. Update image and image-otel to use the shared implementation
so common build arguments are defined in one place.
In `@tests/server.py`:
- Around line 112-118: The _wait_events method uses the misleading parameter
name skip_oneof_names even though it now contains EventType enum members. Rename
this parameter to reflect its EventType-based contents and update all references
and call sites consistently.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: cb2b7231-ac74-49c6-9cde-b6560e75c91e
📒 Files selected for processing (25)
ContainerfileMakefiletests/Makefiletests/conftest.pytests/event.pytests/requirements.txttests/server.pytests/test_acl.pytests/test_config_hotreload.pytests/test_editors/test_nvim.pytests/test_editors/test_sed.pytests/test_editors/test_vi.pytests/test_editors/test_vim.pytests/test_file_open.pytests/test_misc.pytests/test_path_chmod.pytests/test_path_chown.pytests/test_path_mkdir.pytests/test_path_rename.pytests/test_path_rmdir.pytests/test_path_unlink.pytests/test_rate_limit.pytests/test_wildcard.pytests/test_xattr.pytests/utils.py
✅ Files skipped from review due to trivial changes (9)
- tests/requirements.txt
- tests/test_rate_limit.py
- tests/test_misc.py
- tests/test_path_mkdir.py
- tests/test_editors/test_vim.py
- tests/test_xattr.py
- tests/test_path_chmod.py
- tests/test_acl.py
- tests/test_path_unlink.py
🚧 Files skipped from review as they are similar to previous changes (13)
- Containerfile
- tests/test_path_rmdir.py
- tests/test_editors/test_sed.py
- tests/Makefile
- tests/test_editors/test_nvim.py
- tests/test_file_open.py
- tests/test_path_chown.py
- tests/test_editors/test_vi.py
- tests/test_wildcard.py
- tests/test_config_hotreload.py
- tests/test_path_rename.py
- tests/utils.py
- tests/event.py
Molter73
force-pushed
the
mauro/tests/otel-support
branch
from
16f5d71 to
31f8ee3
Compare
Extend the test infrastructure to support both gRPC and OTLP output modes. Both servers translate their native message format into the test's Event/Process types, keeping protocol-specific code isolated. Changes: - Add EventServer base class with shared queue/wait logic - Add GrpcServer translating FileActivity protobufs into Events - Add OtlpServer receiving OTLP/HTTP binary protobuf log exports - Refactor Event.diff() and Process.diff() to compare Event vs Event - Add --output pytest option (grpc, otlp, all; default: grpc) - Parameterize server fixture so tests run per output mode - Add pytest-otlp and pytest-all Makefile targets - Fix rust_style_quote to match Rust shlex backslash handling - Add opentelemetry-proto dependency - Add CARGO_ARGS build arg to Containerfile - Add image-otel Makefile target Assisted-by: claude-opus-4-6@default <noreply@opencode.ai>
Molter73
force-pushed
the
mauro/tests/otel-support
branch
from
31f8ee3 to
cbf9c70
Compare
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
3 participants
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.
Description
Extend the test infrastructure to support both gRPC and OTLP output modes. Both servers translate their native message format into the test's Event/Process types, keeping protocol-specific code isolated.
Changes:
Assisted-by: claude-opus-4-6@default noreply@opencode.ai
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
Run the tests manually + added them to CI in a follow up (#1013)
Summary by CodeRabbit