CodeRabbit Generated Unit Tests: Add unit tests for PR changes#1865
Closed
coderabbitai[bot] wants to merge 2 commits intomainfrom
Closed
CodeRabbit Generated Unit Tests: Add unit tests for PR changes#1865coderabbitai[bot] wants to merge 2 commits intomainfrom
coderabbitai[bot] wants to merge 2 commits intomainfrom
Conversation
Contributor
Author
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 Coding Plan
Comment |
for more information, see https://pre-commit.ci
|
Contributor
Greptile SummaryThis PR adds a new test file ( Key issues found:
Confidence Score: 2/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[pytest collects test_dockerfiles.py] --> B[read_dockerfile: load Dockerfile text]
B --> C{Test category}
C --> D[Negative assertions\nARG VERSION absent\nversion= label absent]
C --> E[Positive assertions\nmaintainer label present\ndescription label present]
C --> F[Structural assertions\nFROM AS base image correct\nFile exists]
D --> G[get_base_stage_content\nextract FROM...AS base block]
E --> G
G --> H[get_label_block_keys\nextract LABEL keys via regex]
H --> I{Single-line LABEL?}
I -- Yes --> J[⚠️ Next instruction line\nappended before break\nlatent false-key bug]
I -- No, multi-line --> K[Correctly terminates\non missing backslash]
D --> L{Current Dockerfiles\nhave ARG VERSION=dev?}
L -- Yes --> M[❌ Tests FAIL on main\nbefore PR-1864 merges]
L -- No after PR-1864 --> N[✅ Tests PASS]
Prompt To Fix All With AIThis is a comment left during a code review.
Path: docker/tests/test_dockerfiles.py
Line: 112-153
Comment:
**Tests will fail against current Dockerfiles**
The tests assert the *post-PR-#1864* state (i.e., `ARG VERSION` and `version=` label are absent), but the actual Dockerfiles in `docker/dockerfiles/` still contain both of those. For example, `backend.Dockerfile` currently has:
```
ARG VERSION=dev
LABEL maintainer="Zipstack Inc." \
description="Backend Service Container" \
version="${VERSION}"
```
This means the following tests will **immediately fail** for all 7 Dockerfiles when run against the current `main` branch:
- `test_no_arg_version_in_dockerfile`
- `test_no_version_label_in_base_stage`
- `test_no_version_arg_default_dev`
- `test_no_version_label_value_interpolation`
These tests should be committed together with (or after) PR #1864 that actually removes `ARG VERSION` from the Dockerfiles — not independently on top of `main`.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: docker/tests/test_dockerfiles.py
Line: 92-99
Comment:
**`get_label_block_keys` can include the next instruction's content**
When a LABEL uses only a single key on one line (not backslash-continued), the function appends the *next* instruction's line before breaking. For example, given:
```dockerfile
LABEL maintainer="Zipstack Inc."
ENV DJANGO_SETTINGS_MODULE="backend.settings.dev"
```
1. The `LABEL` line is appended; the second outer `if` sees it matches `^LABEL\b` so doesn't break.
2. The `ENV` line enters `elif in_label:` and is appended.
3. `label_lines[-2]` (the LABEL line) doesn't end with `\\` → break.
4. But the `ENV` line is now in `label_content`, and `DJANGO_SETTINGS_MODULE="backend.settings.dev"` matches `\b(\w+)\s*=\s*"[^"]*"` — so `DJANGO_SETTINGS_MODULE` would be returned as a label key.
This doesn't affect the current Dockerfiles (which all use multi-line backslash-continued LABELs), but it is a latent bug. The inner `elif` break on `label_lines[-2]` is a safety net for "we've already passed the LABEL block", but it fires one line too late. Consider guarding upfront instead:
```python
elif in_label:
# If previous line didn't end with continuation, we've left the LABEL block
if not label_lines[-1].endswith("\\"):
break
label_lines.append(stripped)
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: docker/tests/test_dockerfiles.py
Line: 239-250
Comment:
**Test with default parameter is redundant and pytest-fragile**
`test_version_arg_removed_not_merely_commented` uses a default function parameter (`dockerfile_name: str = "backend.Dockerfile"`) to act as a standalone test. Pytest will use the default value since `dockerfile_name` is not a registered fixture, so collection won't crash — but this is an unusual pattern that some linters and pytest plugins may flag.
More importantly, this test is almost entirely redundant with `test_no_arg_version_in_dockerfile`, which already covers all 7 Dockerfiles with `@pytest.mark.parametrize`. The only difference is that this version strips comment lines first, but since `test_no_arg_version_in_dockerfile` also uses `re.MULTILINE` and comment lines would not match `^\s*ARG\s+VERSION\b` anyway (a comment line starts with `#`, not whitespace followed by `ARG`), the comment-stripping step adds no meaningful extra coverage.
Consider removing this test, or replacing it with a parametrized version that covers all Dockerfiles if the comment-stripping logic is intentional.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: docker/tests/test_dockerfiles.py
Line: 271-288
Comment:
**`test_worker_unified_distinct_base_image` duplicates `test_base_stage_from_instruction`**
Both `test_base_stage_from_instruction` (parametrized) and `test_worker_unified_distinct_base_image` (also parametrized over all 7 Dockerfiles) verify the exact same thing: the base image in the `FROM ... AS base` instruction for each Dockerfile. The duplicate assertion adds CI overhead (7 extra parametrized test runs) without additional coverage.
Consider removing `test_worker_unified_distinct_base_image` and relying on `test_base_stage_from_instruction` + `EXPECTED_BASE_IMAGES` instead, since that dict already encodes the per-Dockerfile expectations (including `worker-unified`'s distinct `python:3.12.9-slim` image).
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "[pre-commit.ci] auto..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+112
to
+153
| @pytest.mark.parametrize("dockerfile_name", ALL_DOCKERFILES) | ||
| def test_no_arg_version_in_dockerfile(dockerfile_name: str) -> None: | ||
| """Verify that ARG VERSION is not present anywhere in the Dockerfile.""" | ||
| content = read_dockerfile(dockerfile_name) | ||
| # Match any ARG instruction that declares VERSION (with or without default) | ||
| assert not re.search( | ||
| r"^\s*ARG\s+VERSION\b", content, re.MULTILINE | ||
| ), f"{dockerfile_name}: ARG VERSION declaration must not be present (removed in PR)" | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("dockerfile_name", ALL_DOCKERFILES) | ||
| def test_no_version_label_in_base_stage(dockerfile_name: str) -> None: | ||
| """Verify that the version label is not present in the base stage LABEL instruction.""" | ||
| content = read_dockerfile(dockerfile_name) | ||
| base_stage = get_base_stage_content(content) | ||
| # Match 'version=' as a label key (with optional whitespace / backslash continuation) | ||
| assert not re.search( | ||
| r"\bversion\s*=", base_stage, re.IGNORECASE | ||
| ), ( | ||
| f"{dockerfile_name}: 'version=' label must not be present in base stage " | ||
| "(removed in PR)" | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("dockerfile_name", ALL_DOCKERFILES) | ||
| def test_no_version_arg_default_dev(dockerfile_name: str) -> None: | ||
| """Regression: the exact old pattern 'ARG VERSION=dev' must be absent.""" | ||
| content = read_dockerfile(dockerfile_name) | ||
| assert "ARG VERSION=dev" not in content, ( | ||
| f"{dockerfile_name}: The exact old pattern 'ARG VERSION=dev' " | ||
| "must have been removed by this PR" | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("dockerfile_name", ALL_DOCKERFILES) | ||
| def test_no_version_label_value_interpolation(dockerfile_name: str) -> None: | ||
| """Regression: the exact old label value 'version=\"${VERSION}\"' must be absent.""" | ||
| content = read_dockerfile(dockerfile_name) | ||
| assert 'version="${VERSION}"' not in content, ( | ||
| f"{dockerfile_name}: The old label 'version=\"${{VERSION}}\"' " | ||
| "must have been removed by this PR" | ||
| ) |
Contributor
There was a problem hiding this comment.
The tests assert the post-PR-#1864 state (i.e., ARG VERSION and version= label are absent), but the actual Dockerfiles in docker/dockerfiles/ still contain both of those. For example, backend.Dockerfile currently has:
ARG VERSION=dev
LABEL maintainer="Zipstack Inc." \
description="Backend Service Container" \
version="${VERSION}"
This means the following tests will immediately fail for all 7 Dockerfiles when run against the current main branch:
test_no_arg_version_in_dockerfiletest_no_version_label_in_base_stagetest_no_version_arg_default_devtest_no_version_label_value_interpolation
These tests should be committed together with (or after) PR #1864 that actually removes ARG VERSION from the Dockerfiles — not independently on top of main.
Prompt To Fix With AI
This is a comment left during a code review.
Path: docker/tests/test_dockerfiles.py
Line: 112-153
Comment:
**Tests will fail against current Dockerfiles**
The tests assert the *post-PR-#1864* state (i.e., `ARG VERSION` and `version=` label are absent), but the actual Dockerfiles in `docker/dockerfiles/` still contain both of those. For example, `backend.Dockerfile` currently has:
```
ARG VERSION=dev
LABEL maintainer="Zipstack Inc." \
description="Backend Service Container" \
version="${VERSION}"
```
This means the following tests will **immediately fail** for all 7 Dockerfiles when run against the current `main` branch:
- `test_no_arg_version_in_dockerfile`
- `test_no_version_label_in_base_stage`
- `test_no_version_arg_default_dev`
- `test_no_version_label_value_interpolation`
These tests should be committed together with (or after) PR #1864 that actually removes `ARG VERSION` from the Dockerfiles — not independently on top of `main`.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+92
to
+99
| elif in_label: | ||
| label_lines.append(stripped) | ||
| # Continuation ends when the previous line does NOT end with backslash | ||
| if not label_lines[-2].endswith("\\"): | ||
| break | ||
| if in_label and label_lines and not label_lines[-1].endswith("\\"): | ||
| if len(label_lines) >= 1 and not re.match(r"^LABEL\b", label_lines[-1], re.IGNORECASE): | ||
| break |
Contributor
There was a problem hiding this comment.
get_label_block_keys can include the next instruction's content
When a LABEL uses only a single key on one line (not backslash-continued), the function appends the next instruction's line before breaking. For example, given:
LABEL maintainer="Zipstack Inc."
ENV DJANGO_SETTINGS_MODULE="backend.settings.dev"- The
LABELline is appended; the second outerifsees it matches^LABEL\bso doesn't break. - The
ENVline enterselif in_label:and is appended. label_lines[-2](the LABEL line) doesn't end with\\→ break.- But the
ENVline is now inlabel_content, andDJANGO_SETTINGS_MODULE="backend.settings.dev"matches\b(\w+)\s*=\s*"[^"]*"— soDJANGO_SETTINGS_MODULEwould be returned as a label key.
This doesn't affect the current Dockerfiles (which all use multi-line backslash-continued LABELs), but it is a latent bug. The inner elif break on label_lines[-2] is a safety net for "we've already passed the LABEL block", but it fires one line too late. Consider guarding upfront instead:
elif in_label:
# If previous line didn't end with continuation, we've left the LABEL block
if not label_lines[-1].endswith("\\"):
break
label_lines.append(stripped)Prompt To Fix With AI
This is a comment left during a code review.
Path: docker/tests/test_dockerfiles.py
Line: 92-99
Comment:
**`get_label_block_keys` can include the next instruction's content**
When a LABEL uses only a single key on one line (not backslash-continued), the function appends the *next* instruction's line before breaking. For example, given:
```dockerfile
LABEL maintainer="Zipstack Inc."
ENV DJANGO_SETTINGS_MODULE="backend.settings.dev"
```
1. The `LABEL` line is appended; the second outer `if` sees it matches `^LABEL\b` so doesn't break.
2. The `ENV` line enters `elif in_label:` and is appended.
3. `label_lines[-2]` (the LABEL line) doesn't end with `\\` → break.
4. But the `ENV` line is now in `label_content`, and `DJANGO_SETTINGS_MODULE="backend.settings.dev"` matches `\b(\w+)\s*=\s*"[^"]*"` — so `DJANGO_SETTINGS_MODULE` would be returned as a label key.
This doesn't affect the current Dockerfiles (which all use multi-line backslash-continued LABELs), but it is a latent bug. The inner `elif` break on `label_lines[-2]` is a safety net for "we've already passed the LABEL block", but it fires one line too late. Consider guarding upfront instead:
```python
elif in_label:
# If previous line didn't end with continuation, we've left the LABEL block
if not label_lines[-1].endswith("\\"):
break
label_lines.append(stripped)
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+239
to
+250
| def test_version_arg_removed_not_merely_commented(dockerfile_name: str = "backend.Dockerfile") -> None: | ||
| """Regression: the VERSION ARG is not present even as a comment.""" | ||
| content = read_dockerfile(dockerfile_name) | ||
| # The arg must not appear as an active instruction anywhere | ||
| active_lines = [ | ||
| line for line in content.splitlines() if not line.strip().startswith("#") | ||
| ] | ||
| active_content = "\n".join(active_lines) | ||
| assert not re.search(r"^\s*ARG\s+VERSION\b", active_content, re.MULTILINE), ( | ||
| "backend.Dockerfile: ARG VERSION must not appear as an active (non-comment) " | ||
| "instruction" | ||
| ) |
Contributor
There was a problem hiding this comment.
test_version_arg_removed_not_merely_commented uses a default function parameter (dockerfile_name: str = "backend.Dockerfile") to act as a standalone test. Pytest will use the default value since dockerfile_name is not a registered fixture, so collection won't crash — but this is an unusual pattern that some linters and pytest plugins may flag.
More importantly, this test is almost entirely redundant with test_no_arg_version_in_dockerfile, which already covers all 7 Dockerfiles with @pytest.mark.parametrize. The only difference is that this version strips comment lines first, but since test_no_arg_version_in_dockerfile also uses re.MULTILINE and comment lines would not match ^\s*ARG\s+VERSION\b anyway (a comment line starts with #, not whitespace followed by ARG), the comment-stripping step adds no meaningful extra coverage.
Consider removing this test, or replacing it with a parametrized version that covers all Dockerfiles if the comment-stripping logic is intentional.
Prompt To Fix With AI
This is a comment left during a code review.
Path: docker/tests/test_dockerfiles.py
Line: 239-250
Comment:
**Test with default parameter is redundant and pytest-fragile**
`test_version_arg_removed_not_merely_commented` uses a default function parameter (`dockerfile_name: str = "backend.Dockerfile"`) to act as a standalone test. Pytest will use the default value since `dockerfile_name` is not a registered fixture, so collection won't crash — but this is an unusual pattern that some linters and pytest plugins may flag.
More importantly, this test is almost entirely redundant with `test_no_arg_version_in_dockerfile`, which already covers all 7 Dockerfiles with `@pytest.mark.parametrize`. The only difference is that this version strips comment lines first, but since `test_no_arg_version_in_dockerfile` also uses `re.MULTILINE` and comment lines would not match `^\s*ARG\s+VERSION\b` anyway (a comment line starts with `#`, not whitespace followed by `ARG`), the comment-stripping step adds no meaningful extra coverage.
Consider removing this test, or replacing it with a parametrized version that covers all Dockerfiles if the comment-stripping logic is intentional.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+271
to
+288
| @pytest.mark.parametrize("dockerfile_name", ALL_DOCKERFILES) | ||
| def test_worker_unified_distinct_base_image(dockerfile_name: str) -> None: | ||
| """Verify worker-unified uses python:3.12.9-slim while others use python:3.12-slim-trixie.""" | ||
| content = read_dockerfile(dockerfile_name) | ||
| match = re.search( | ||
| r"^FROM\s+(\S+)\s+AS\s+base\b", content, re.MULTILINE | re.IGNORECASE | ||
| ) | ||
| assert match is not None | ||
| actual_image = match.group(1) | ||
| if dockerfile_name == "worker-unified.Dockerfile": | ||
| assert actual_image == "python:3.12.9-slim", ( | ||
| "worker-unified.Dockerfile must use python:3.12.9-slim as base image" | ||
| ) | ||
| else: | ||
| assert actual_image == "python:3.12-slim-trixie", ( | ||
| f"{dockerfile_name}: non-worker-unified Dockerfiles must use " | ||
| "python:3.12-slim-trixie as base image" | ||
| ) |
Contributor
There was a problem hiding this comment.
test_worker_unified_distinct_base_image duplicates test_base_stage_from_instruction
Both test_base_stage_from_instruction (parametrized) and test_worker_unified_distinct_base_image (also parametrized over all 7 Dockerfiles) verify the exact same thing: the base image in the FROM ... AS base instruction for each Dockerfile. The duplicate assertion adds CI overhead (7 extra parametrized test runs) without additional coverage.
Consider removing test_worker_unified_distinct_base_image and relying on test_base_stage_from_instruction + EXPECTED_BASE_IMAGES instead, since that dict already encodes the per-Dockerfile expectations (including worker-unified's distinct python:3.12.9-slim image).
Prompt To Fix With AI
This is a comment left during a code review.
Path: docker/tests/test_dockerfiles.py
Line: 271-288
Comment:
**`test_worker_unified_distinct_base_image` duplicates `test_base_stage_from_instruction`**
Both `test_base_stage_from_instruction` (parametrized) and `test_worker_unified_distinct_base_image` (also parametrized over all 7 Dockerfiles) verify the exact same thing: the base image in the `FROM ... AS base` instruction for each Dockerfile. The duplicate assertion adds CI overhead (7 extra parametrized test runs) without additional coverage.
Consider removing `test_worker_unified_distinct_base_image` and relying on `test_base_stage_from_instruction` + `EXPECTED_BASE_IMAGES` instead, since that dict already encodes the per-Dockerfile expectations (including `worker-unified`'s distinct `python:3.12.9-slim` image).
How can I resolve this? If you propose a fix, please make it concise.
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.
Unit test generation was requested by @ritwik-g.
The following files were modified:
docker/tests/test_dockerfiles.py