tests: re-order python-basic-tests to run cheap tests first#453
tests: re-order python-basic-tests to run cheap tests first#453
Conversation
WalkthroughUpdates GitHub Actions workflow to run code-quality checks earlier and only once, with parallelized flake8/pylint. Removes duplicated later checks. Adds a pycodestyle configuration to setup.cfg. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Push/PR
participant GHA as GitHub Actions
participant Job as python-basic-tests job
Note over Dev,GHA: CI pipeline control-flow change
Dev->>GHA: Trigger workflow
GHA->>Job: Start job
Job->>Job: Create boom configuration
rect rgba(198,239,206,0.4)
Note right of Job: Early code-quality checks (new position)
Job->>Job: Run pycodestyle (cfg-driven)
Job->>Job: Run flake8 --jobs auto
Job->>Job: Run pylint --jobs 0
Job->>Job: Run bandit with skips
end
Job->>Job: Install stratis-cli (later)
Job->>Job: Continue remaining tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (45)
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/snapm.yml (3)
56-67: Run linters even earlier to shave more time.Consider moving these four steps to just after “Install Ubuntu snapm deps” (and before pip install, package install, and any systemd copies/reload). If pylint import resolution requires deps, use pylint’s import-error disable or PYTHONPATH to still avoid the install/reload overhead.
56-67: Minor speed-ups: enable parallelism for flake8/pylint.Add flake8’s auto-parallel and pylint’s job auto-detection to reduce wall time.
- - name: Check Flake8 + - name: Check Flake8 run: > - flake8 snapm + flake8 --jobs auto snapm - - name: Check pylint + - name: Check pylint run: > - pylint snapm + pylint -j 0 snapm
56-67: Centralize linter config to keep CI/local consistent.Instead of passing ignores on the CLI (pycodestyle E501,E203,W503 and Bandit skip list), consider moving them into pyproject.toml/setup.cfg. Then these steps can omit flags and devs get the same behavior locally.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/snapm.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-26T15:22:27.918Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#383
File: virt_tests/testvm.py:493-499
Timestamp: 2025-08-26T15:22:27.918Z
Learning: In the snapm virt_tests testing workflow, the target distributions (Fedora 41/42, CentOS Stream 9/10) all provide the bare `pip` executable when python3-pip is installed, so using `pip install` instead of `python3 -m pip install` is acceptable and works reliably across these specific distributions.
Applied to files:
.github/workflows/snapm.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, fedora41)
- GitHub Check: python-basic-tests
🔇 Additional comments (1)
.github/workflows/snapm.yml (1)
56-67: Good reorder: fast-fail linters now run before Stratis build/install.This meets the goal of surfacing basic issues earlier and removes redundant later checks. Nice.
bmr-cymru
force-pushed
the
bmr-reorder-python-basic-tests
branch
2 times, most recently
from
d5eb47a to
76f4c2c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pyproject.toml (1)
30-32: Document skip rationale and consider scoping targets.Add brief TOML comments explaining why B101/B303/B404/B603 are skipped, and optionally set
targets = ["snapm"]so localbandit -c pyproject.tomlworks without extra args.[tool.bandit] +# Rationale: skip noisy checks for our codebase; review periodically. exclude_dirs = ["tests", "container_tests", "virt_tests"] skips = ["B101", "B303", "B404", "B603"] +targets = ["snapm"].github/workflows/snapm.yml (3)
25-40: Trim duplicate packages to shave CI time.You install both
python3-pycodestyleandpycodestyle, andpython3-flake8andflake8. Keep one of each (the Python 3 packages are sufficient on Ubuntu) to reduce apt work.python3-pycodestyle - pycodestyle libfaketime faketime - flake8 pylint bandit
41-52: Minor:--jobs autofor flake8 is redundant.
autois already the default; you can drop it for brevity. (flake8.pycqa.org)- flake8 --jobs auto snapm + flake8 snapm
21-40: Run linters before heavier deps to fail even earlier.Split the apt install: first just linters, run checks, then install the rest (e.g., lvm2). This reduces wasted setup when a quick check fails.
- name: Run apt-get update run: sudo apt-get -q update - - name: Install Ubuntu snapm deps + - name: Install linter deps env: DEBIAN_FRONTEND: noninteractive run: > sudo apt-get install -y - python3-pytest - python3-pytest-subtests - python3-pycodestyle - python3-coverage - python3-flake8 - python3-dateutil - python3-packaging - python3-wcwidth - pycodestyle - libfaketime - faketime - flake8 - pylint - bandit - lvm2 + python3-pycodestyle python3-flake8 pylint bandit + - name: Check PyCodestyle run: > pycodestyle snapm - name: Check Flake8 run: > - flake8 --jobs auto snapm + flake8 snapm - name: Check pylint run: > pylint --jobs 0 snapm - name: Run bandit run: > bandit -c pyproject.toml -r snapm + - name: Install remaining Ubuntu deps + env: + DEBIAN_FRONTEND: noninteractive + run: > + sudo apt-get install -y + python3-pytest python3-pytest-subtests python3-coverage + python3-dateutil python3-packaging python3-wcwidth + libfaketime faketime lvm2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/snapm.yml(1 hunks)pyproject.toml(1 hunks)setup.cfg(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- setup.cfg
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-26T15:22:27.918Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#383
File: virt_tests/testvm.py:493-499
Timestamp: 2025-08-26T15:22:27.918Z
Learning: In the snapm virt_tests testing workflow, the target distributions (Fedora 41/42, CentOS Stream 9/10) all provide the bare `pip` executable when python3-pip is installed, so using `pip install` instead of `python3 -m pip install` is acceptable and works reliably across these specific distributions.
Applied to files:
.github/workflows/snapm.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (37)
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, fedora41)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, fedora41)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
🔇 Additional comments (3)
pyproject.toml (1)
30-32: Bandit config centralization LGTM.Using [tool.bandit] with exclude_dirs and skips is correct and aligns CI with a single source of truth. (bandit.readthedocs.io)
.github/workflows/snapm.yml (2)
41-52: Fast‑fail order and parallel linting look good.Running pycodestyle/flake8/pylint/bandit early achieves the “fail fast” goal;
pylint --jobs 0auto-detects CPUs, and flake8 supports parallel--jobs. (pylint.pycqa.org, flake8.pycqa.org)
47-49: Watch for pylint parallelism quirks.Historically,
-j 0(auto) could yield different reports vs. serialized runs in some environments. If you see flaky results, pin--jobs 1. (github.com)
|
@coderabbitai re: "Run linters even earlier to shave more time" So no, let's not do that. |
|
Tip For best results, initiate chat on the files or code changes. @bmr-cymru Thanks for testing that! You're absolutely right - running pylint before the dependencies are installed causes those import errors since pylint needs to be able to import the modules it's analyzing. The current approach in your PR strikes the right balance by moving the linters after the basic Ubuntu package installation (which includes Python linting tools) but before the expensive Stratis component installation. This gives us the "fail fast" benefit without the dependency issues. I also notice there's a pylintrc configuration warning about an unknown option 'R0917'. That might be worth addressing separately, but it doesn't affect the core objective of this PR. The reordering you've implemented looks good - it achieves the goal of failing faster on basic code quality issues without the complications of missing dependencies. |
bmr-cymru
force-pushed
the
bmr-reorder-python-basic-tests
branch
from
76f4c2c to
61fc424
Compare
That's because the version in CI is old. It's harmless. |
|
Tip For best results, initiate chat on the files or code changes. @bmr-cymru Thanks for the clarification! That makes perfect sense - version differences between the pylint in CI and the configuration can definitely cause those harmless warnings about unknown options. Good to know that's expected behavior and not something that needs fixing. Your testing and the current approach in this PR definitely validate that the reordering strategy you've chosen is the right one - getting the code quality checks to run early without running into the dependency maze. ✏️ Learnings added
|
|
@coderabbitai: re: "Centralize linter configs" No workie for |
|
Tip For best results, initiate chat on the files or code changes. . The most compatible approach would be to either:
Given that the CI environment constraints make the centralized config approach problematic, I'd recommend sticking with the current approach of using command-line arguments for bandit configuration in the workflow. This keeps things working reliably across different bandit versions without additional dependencies. The current PR approach is solid - moving the linters to run earlier while keeping them compatible with the CI environment is exactly the right balance! 🧠 Learnings used |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Resolves: #457 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
bmr-cymru
force-pushed
the
bmr-reorder-python-basic-tests
branch
from
c58bee0 to
d86e8fa
Compare
1 participant
Resolves: #310
Resolves: #456
Resolves: #457
Summary by CodeRabbit
Chores
Style
Tests