Conversation
Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
|
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:
📝 WalkthroughWalkthroughConsolidates test execution under a root tox, renames/moves the daily-tests workdir, adds Codecov upload steps and a codecov.yml, expands .gitignore for coverage artifacts, removes package-level pytest/tox configs, and updates imports and test patch targets to package-qualified paths. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as "GitHub Actions\n(runner)"
participant Tox as "tox\n(test env)"
participant Pytest as "pytest\n(+pytest-cov)"
participant Repo as "Repository\n(files & coverage)"
participant Codecov as "Codecov\n(codecov-action@v5)"
GH->>Repo: checkout
GH->>Tox: run tox env (daily-tests / ocp-stream-generator)
Tox->>Pytest: invoke pytest --cov --cov-report=xml:coverage.xml
Pytest->>Repo: write coverage.xml (package dir)
GH->>Codecov: upload coverage XML with flag & token
Note right of Codecov: fail_ci_if_error: false
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
ocp-stream-generator/tests/test_json_builder.py (1)
4-15: LGTM! Consider consolidating imports for readability.The absolute import paths are correct and will ensure proper module resolution during test execution with coverage.
Optionally, the imports from the same module could be consolidated:
♻️ Optional: Consolidate imports
-from ocp_stream_generator.stream_generator import JsonBuilder -from ocp_stream_generator.stream_generator import Tag -from ocp_stream_generator.stream_generator import LatestTag -from ocp_stream_generator.stream_generator import CustomTag -from ocp_stream_generator.stream_generator import ImagestreamFile -from tests.data.data_json_builder import add_tag_result -from tests.data.data_json_builder import add_tag_latest_result -from tests.data.data_json_builder import add_tag_custom_result -from tests.data.data_json_builder import create_annotation_result -from tests.data.data_json_builder import create_annotation_latest_result -from tests.data.data_json_builder import create_header_result -from tests.data.data_json_builder import generate_json_result +from ocp_stream_generator.stream_generator import ( + JsonBuilder, + Tag, + LatestTag, + CustomTag, + ImagestreamFile, +) +from tests.data.data_json_builder import ( + add_tag_result, + add_tag_latest_result, + add_tag_custom_result, + create_annotation_result, + create_annotation_latest_result, + create_header_result, + generate_json_result, +)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ocp-stream-generator/tests/test_json_builder.py` around lines 4 - 15, Consolidate multiple same-module imports to improve readability: replace the repeated imports of JsonBuilder, Tag, LatestTag, CustomTag, and ImagestreamFile from ocp_stream_generator.stream_generator with a single grouped import statement listing those symbols, and likewise replace the multiple imports from tests.data.data_json_builder (add_tag_result, add_tag_latest_result, add_tag_custom_result, create_annotation_result, create_annotation_latest_result, create_header_result, generate_json_result) with one grouped import; keep the exact symbol names unchanged so tests and references to JsonBuilder, Tag, LatestTag, CustomTag, ImagestreamFile and the seven test data functions continue to resolve correctly..github/workflows/python-daily-tests.yml (2)
18-18: Consider updatingactions/checkoutto v4.Same as the ocp-stream-generator workflow:
actions/checkout@v2is outdated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/python-daily-tests.yml at line 18, Replace the outdated actions/checkout@v2 usage in the workflow with the newer actions/checkout@v4; locate the line containing "uses: actions/checkout@v2" and update the version to "@v4" so the workflow uses the latest checkout action.
23-29: Consider conditional upload on test success.Same consideration as the ocp-stream-generator workflow: add
if: success()to avoid upload attempts when tests fail.♻️ Suggested improvement
- name: Upload coverage to Codecov + if: success() uses: codecov/codecov-action@v5 with: token: ${{ secrets.CODECOV_TOKEN }} flags: daily-tests-unit files: daily_tests/coverage.xml fail_ci_if_error: false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/python-daily-tests.yml around lines 23 - 29, The Codecov upload step ("Upload coverage to Codecov" using codecov/codecov-action@v5) should be run only when tests succeed; add the conditional `if: success()` to that step so the upload is skipped on failed test runs, preventing unnecessary/erroneous upload attempts..github/workflows/python-ocp-generator-tests.yml (2)
18-18: Consider updatingactions/checkoutto v4.
actions/checkout@v2is significantly outdated. The current stable version is v4, which includes performance improvements and security fixes.♻️ Suggested update
- - uses: actions/checkout@v2 + - uses: actions/checkout@v4🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/python-ocp-generator-tests.yml at line 18, Update the GitHub Actions checkout step to use the current stable release by replacing the reference to actions/checkout@v2 with actions/checkout@v4; locate the workflow step that uses "actions/checkout@v2" and change it to "actions/checkout@v4" to pick up performance and security fixes, then commit the workflow change and verify CI runs successfully.
23-29: Consider conditional upload on test success.The Codecov upload step will run regardless of test outcome. If tests fail, the coverage file may be missing or incomplete, causing upload errors (though mitigated by
fail_ci_if_error: false).♻️ Suggested improvement
- name: Upload coverage to Codecov + if: success() uses: codecov/codecov-action@v5 with: token: ${{ secrets.CODECOV_TOKEN }} flags: ocp-stream-generator-unit files: ocp-stream-generator/coverage.xml fail_ci_if_error: false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/python-ocp-generator-tests.yml around lines 23 - 29, The Codecov upload step ("Upload coverage to Codecov" using codecov/codecov-action@v5) runs even when tests fail; make the upload conditional by adding an if: ${{ success() }} (or if: ${{ needs.<job>.result == 'success' }} when in a multi-job workflow) to the step so it only executes when prior test steps succeed, and keep fail_ci_if_error: false as a safeguard.codecov.yml (1)
9-13: Consider addingpathsto flags for correct coverage attribution.Without
pathsconfiguration, Codecov may not correctly attribute coverage to each flag. When using flags for separate components in a monorepo, specifying paths helps Codecov understand which files belong to which coverage flag.♻️ Suggested improvement
flags: daily-tests-unit: carryforward: true + paths: + - daily_tests/ ocp-stream-generator-unit: carryforward: true + paths: + - ocp-stream-generator/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codecov.yml` around lines 9 - 13, The flags block is missing paths so Codecov can't attribute coverage per component; update the flags for daily-tests-unit and ocp-stream-generator-unit to include a paths array that lists the repository directories/files belonging to each flag (e.g., the package or src folders for the daily tests component and the ocp-stream-generator component), ensuring each flag (daily-tests-unit, ocp-stream-generator-unit) has an explicit paths: entry to correctly scope coverage to those files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/python-daily-tests.yml:
- Line 18: Replace the outdated actions/checkout@v2 usage in the workflow with
the newer actions/checkout@v4; locate the line containing "uses:
actions/checkout@v2" and update the version to "@v4" so the workflow uses the
latest checkout action.
- Around line 23-29: The Codecov upload step ("Upload coverage to Codecov" using
codecov/codecov-action@v5) should be run only when tests succeed; add the
conditional `if: success()` to that step so the upload is skipped on failed test
runs, preventing unnecessary/erroneous upload attempts.
In @.github/workflows/python-ocp-generator-tests.yml:
- Line 18: Update the GitHub Actions checkout step to use the current stable
release by replacing the reference to actions/checkout@v2 with
actions/checkout@v4; locate the workflow step that uses "actions/checkout@v2"
and change it to "actions/checkout@v4" to pick up performance and security
fixes, then commit the workflow change and verify CI runs successfully.
- Around line 23-29: The Codecov upload step ("Upload coverage to Codecov" using
codecov/codecov-action@v5) runs even when tests fail; make the upload
conditional by adding an if: ${{ success() }} (or if: ${{ needs.<job>.result ==
'success' }} when in a multi-job workflow) to the step so it only executes when
prior test steps succeed, and keep fail_ci_if_error: false as a safeguard.
In `@codecov.yml`:
- Around line 9-13: The flags block is missing paths so Codecov can't attribute
coverage per component; update the flags for daily-tests-unit and
ocp-stream-generator-unit to include a paths array that lists the repository
directories/files belonging to each flag (e.g., the package or src folders for
the daily tests component and the ocp-stream-generator component), ensuring each
flag (daily-tests-unit, ocp-stream-generator-unit) has an explicit paths: entry
to correctly scope coverage to those files.
In `@ocp-stream-generator/tests/test_json_builder.py`:
- Around line 4-15: Consolidate multiple same-module imports to improve
readability: replace the repeated imports of JsonBuilder, Tag, LatestTag,
CustomTag, and ImagestreamFile from ocp_stream_generator.stream_generator with a
single grouped import statement listing those symbols, and likewise replace the
multiple imports from tests.data.data_json_builder (add_tag_result,
add_tag_latest_result, add_tag_custom_result, create_annotation_result,
create_annotation_latest_result, create_header_result, generate_json_result)
with one grouped import; keep the exact symbol names unchanged so tests and
references to JsonBuilder, Tag, LatestTag, CustomTag, ImagestreamFile and the
seven test data functions continue to resolve correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6793df3d-05ce-4c8f-b30b-3f5da91fb8fc
📒 Files selected for processing (16)
.github/workflows/python-daily-tests.yml.github/workflows/python-ocp-generator-tests.yml.gitignorecodecov.ymldaily_tests/pytest.inidaily_tests/tests/integration/__init__.pydaily_tests/tests/integration/test_integration_placeholder.pydaily_tests/tox.iniocp-stream-generator/ocp_stream_generator/stream_generator.pyocp-stream-generator/pytest.iniocp-stream-generator/tests/__init__.pyocp-stream-generator/tests/test_imagestream_file.pyocp-stream-generator/tests/test_json_builder.pyocp-stream-generator/tests/test_yaml_loader.pyocp-stream-generator/tox.initox.ini
💤 Files with no reviewable changes (3)
- daily_tests/pytest.ini
- daily_tests/tox.ini
- ocp-stream-generator/pytest.ini
like in container-ci-suite Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tox.ini`:
- Around line 4-11: The CI expects the coverage report under
daily_tests/coverage.xml but the [testenv:daily_tests] block currently runs from
repo root; either add changedir = daily_tests to the testenv:daily_tests section
so tests run from the daily_tests directory, or modify the commands line (the
--cov-report argument) to --cov-report=xml:daily_tests/coverage.xml so the
generated coverage.xml is placed where CI expects; update the commands or add
changedir accordingly (see testenv:daily_tests, commands, changedir, and
--cov-report).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da07f67a-a471-4752-adac-78cc4c1b0dd1
📒 Files selected for processing (2)
.gitignoretox.ini
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
daily-tests/tests/test_tf_log_downloader.py (1)
8-11: Consider reordering or removing the redundantsys.pathmodification.The
sys.path.inserton line 11 executes after the import on line 8 has already succeeded. Ifdaily_testsis importable at line 8, the path modification is redundant. If it's not importable, line 8 would fail before line 11 executes.Since the import works (presumably via pytest/tox configuration), consider removing line 11 or moving it before the imports if it's needed for other purposes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@daily-tests/tests/test_tf_log_downloader.py` around lines 8 - 11, The sys.path modification after importing daily_tests is redundant or misplaced: either remove the sys.path.insert(0, str(TEST_DIR.parent)) line entirely if pytest/tox already makes daily_tests importable, or move that sys.path.insert call to before the import statement so the path is added before executing from daily_tests import download_logs; update the test module to perform the path manipulation prior to any imports that depend on it or delete the insertion if it's unnecessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@daily-tests/tests/test_show_logs.py`:
- Around line 10-13: The package import fails because the package file in the
daily_tests package is misspelled; rename the file named "__init_.py" in the
daily-tests/daily_tests package to the correct "__init__.py" so that "from
daily_tests import show_logs" in tests/test_show_logs.py can import the
daily_tests package and its show_logs module; verify the package now exposes
show_logs and run the test to confirm the import succeeds.
In `@daily-tests/tox.ini`:
- Around line 1-2: Rename the incorrectly named package initializer
daily_tests/__init_.py (currently daily_tests/__init_.py with a single
underscore) to the correct double-underscore filename __init__.py so Python
recognizes the daily_tests package used by the pytest coverage flag
(--cov=daily_tests); verify the file exists at daily_tests/__init__.py (can be
empty or contain package-level imports) and re-run the tox pytest command to
confirm coverage collection works.
---
Nitpick comments:
In `@daily-tests/tests/test_tf_log_downloader.py`:
- Around line 8-11: The sys.path modification after importing daily_tests is
redundant or misplaced: either remove the sys.path.insert(0,
str(TEST_DIR.parent)) line entirely if pytest/tox already makes daily_tests
importable, or move that sys.path.insert call to before the import statement so
the path is added before executing from daily_tests import download_logs; update
the test module to perform the path manipulation prior to any imports that
depend on it or delete the insertion if it's unnecessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87466fa7-b2a1-416c-a780-a6bb31739a1b
📒 Files selected for processing (21)
.github/workflows/python-daily-tests.ymldaily-tests/daily_tests/__init_.pydaily-tests/daily_tests/daily_grades.pydaily-tests/daily_tests/daily_helm_charts.shdaily-tests/daily_tests/daily_nightly_tests_report.pydaily-tests/daily_tests/daily_nightly_tests_report.shdaily-tests/daily_tests/daily_scl_tests.shdaily-tests/daily_tests/download_logs.pydaily-tests/daily_tests/sclorg_sanity_tests.pydaily-tests/daily_tests/show_logs.pydaily-tests/daily_tests/show_logs.shdaily-tests/tests/__init__.pydaily-tests/tests/no_logs_in_results.txtdaily-tests/tests/results.xmldaily-tests/tests/test_daily_grades.pydaily-tests/tests/test_show_logs.pydaily-tests/tests/test_tf_log_downloader.pydaily-tests/tests/tmt_log_outputdaily-tests/tox.inirun_nightly_tests.shtox.ini
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/python-daily-tests.yml
- tox.ini
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/python-daily-tests.yml (1)
18-19: Consider updating to newer action versions.
actions/checkout@v2is deprecated and uses an older Node.js runtime. The current stable version is v4 which includes security improvements and better performance.Similarly, pinning
tox-github-action@mainto a branch rather than a version tag could introduce unexpected breaking changes.🔧 Suggested update
- - uses: actions/checkout@v2 - - uses: fedora-python/tox-github-action@main + - uses: actions/checkout@v4 + - uses: fedora-python/tox-github-action@main # Consider pinning to a specific version tag if available🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/python-daily-tests.yml around lines 18 - 19, Update the workflow to use supported, pinned action versions: replace uses: actions/checkout@v2 with uses: actions/checkout@v4 to get the current stable runtime and security fixes, and change uses: fedora-python/tox-github-action@main to a specific released tag or commit SHA (not a branch) to avoid unexpected breaking changes; ensure the chosen tag is the latest stable release of fedora-python/tox-github-action and run the workflow to verify compatibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/python-daily-tests.yml:
- Around line 18-19: Update the workflow to use supported, pinned action
versions: replace uses: actions/checkout@v2 with uses: actions/checkout@v4 to
get the current stable runtime and security fixes, and change uses:
fedora-python/tox-github-action@main to a specific released tag or commit SHA
(not a branch) to avoid unexpected breaking changes; ensure the chosen tag is
the latest stable release of fedora-python/tox-github-action and run the
workflow to verify compatibility.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25cee31b-632d-4dad-a6ff-68579e7dc731
📒 Files selected for processing (21)
.github/workflows/python-daily-tests.ymldaily-tests/daily_tests/__init__.pydaily-tests/daily_tests/daily_grades.pydaily-tests/daily_tests/daily_helm_charts.shdaily-tests/daily_tests/daily_nightly_tests_report.pydaily-tests/daily_tests/daily_nightly_tests_report.shdaily-tests/daily_tests/daily_scl_tests.shdaily-tests/daily_tests/download_logs.pydaily-tests/daily_tests/sclorg_sanity_tests.pydaily-tests/daily_tests/show_logs.pydaily-tests/daily_tests/show_logs.shdaily-tests/tests/__init__.pydaily-tests/tests/no_logs_in_results.txtdaily-tests/tests/results.xmldaily-tests/tests/test_daily_grades.pydaily-tests/tests/test_show_logs.pydaily-tests/tests/test_tf_log_downloader.pydaily-tests/tests/tmt_log_outputdaily-tests/tox.inirun_nightly_tests.shtox.ini
🚧 Files skipped from review as they are similar to previous changes (4)
- run_nightly_tests.sh
- tox.ini
- daily-tests/tests/test_show_logs.py
- daily-tests/tox.ini
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@daily-tests/tests/test_tf_log_downloader.py`:
- Around line 8-11: The import of daily_tests occurs before modifying sys.path,
so when pytest runs outside the repo the package isn't found; move the sys.path
manipulation (the TEST_DIR and sys.path.insert(0, str(TEST_DIR.parent)) lines)
above the "from daily_tests import download_logs" import (or remove these lines
entirely if your tox config guarantees the package is installed) so that
TEST_DIR/parent is on sys.path before attempting to import download_logs from
daily_tests.
In `@daily-tests/tox.ini`:
- Around line 1-8: Tox is trying to build/install the daily-tests package
(causing failures because there is no packaging metadata); add a [tox] section
with package = skip to the tox.ini to disable packaging. Update the file to
include a top-level [tox] header and set package = skip so tox will not attempt
to build/install the project before running the existing [testenv] commands.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a44fa4ed-46b9-4728-a883-a4c5d3ac5541
📒 Files selected for processing (21)
.github/workflows/python-daily-tests.ymldaily-tests/daily_tests/__init__.pydaily-tests/daily_tests/daily_grades.pydaily-tests/daily_tests/daily_helm_charts.shdaily-tests/daily_tests/daily_nightly_tests_report.pydaily-tests/daily_tests/daily_nightly_tests_report.shdaily-tests/daily_tests/daily_scl_tests.shdaily-tests/daily_tests/download_logs.pydaily-tests/daily_tests/sclorg_sanity_tests.pydaily-tests/daily_tests/show_logs.pydaily-tests/daily_tests/show_logs.shdaily-tests/tests/__init__.pydaily-tests/tests/no_logs_in_results.txtdaily-tests/tests/results.xmldaily-tests/tests/test_daily_grades.pydaily-tests/tests/test_show_logs.pydaily-tests/tests/test_tf_log_downloader.pydaily-tests/tests/tmt_log_outputdaily-tests/tox.inirun_nightly_tests.shtox.ini
🚧 Files skipped from review as they are similar to previous changes (2)
- daily-tests/tests/test_show_logs.py
- tox.ini
so as ocp-stream-generator have the same directory structure Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
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.
Summary by CodeRabbit
CI/CD
Tests
Chores