Skip to content

CI: remove codacy scan and improve tests handling#696

Merged
juarezr merged 11 commits into
petl-developers:masterfrom
juarezr:fix/ci-improve-scan-tests
Jun 3, 2026
Merged

CI: remove codacy scan and improve tests handling#696
juarezr merged 11 commits into
petl-developers:masterfrom
juarezr:fix/ci-improve-scan-tests

Conversation

@juarezr

@juarezr juarezr commented Jun 3, 2026

Copy link
Copy Markdown
Member

This PR aims to improve the existing tests for CI and local executions.

Changes

  1. Removed codacy analysis scan
  2. Improved database connection handling in tests
  3. Fix some python2.7 compatibility in tests and packages
  4. Improved the CI job report

Checklist

Use this checklist to ensure the quality of pull requests that include new code and/or make changes to existing code.

  • Source Code guidelines:
    • Includes unit tests
    • New functions have docstrings with examples that can be run with doctest
    • New functions are included in API docs
    • Docstrings include notes for any changes to API or behavior
    • All changes are documented in docs/changes.rst
  • Versioning and history tracking guidelines:
    • Using atomic commits whenever possible
    • Commits are reversible whenever possible
    • There are no incomplete changes in the pull request
    • There is no accidental garbage added to the source code
  • Testing guidelines:
    • Tested locally using tox / pytest
    • Rebased to master branch and tested before sending the PR
    • Automated testing passes (see CI)
    • Unit test coverage has not decreased (see Coveralls)
  • State of these changes is:
    • Just a proof of concept
    • Work in progress / Further changes needed
    • Ready to review
    • Ready to merge

juarezr added 7 commits June 2, 2026 21:21
Motivation:
- The job is failing due to the lack of multi-tools Sarif support on Github
- We already have CodeQL running as well
- It barely runs python linters like bandit, pylint, ruff
- It runs some non related linters like: pmd, checov, jacksonlinter
- Currently there is configuration for any python linters in the repository
- It outputs too much issues that make it unfeasible
- Nobody is checking the tool outputs
- We have to settle on the tools as described in petl-developers#597
- We can enable it again further
@juarezr juarezr added the Build CI CD Issues affecting github pypi readthedocs coveralls conda-forge label Jun 3, 2026
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Review Summary by Qodo

CI improvements: remove Codacy scan, enhance test handling, and add sandbox containers

✨ Enhancement 🧪 Tests

Grey Divider

Walkthroughs

Description
• Removed Codacy security scan workflow due to multi-tool SARIF support limitations
• Refactored database connection argument handling with environment variable support
• Added Python 3.14 support and improved Python 2.7 compatibility in dependencies
• Enhanced CI workflow reporting with installed packages and improved step ordering
• Added Docker Compose sandbox containers for Python 3.x and Python 2.7 testing
Diagram
flowchart LR
  A["CI Workflow"] -->|"Remove"| B["Codacy Scan"]
  A -->|"Refactor"| C["DB Connection Args"]
  C -->|"Add"| D["Environment Variables"]
  A -->|"Enhance"| E["Test Dependencies"]
  E -->|"Add"| F["Python 3.14"]
  E -->|"Fix"| G["Python 2.7 Compat"]
  A -->|"Improve"| H["Workflow Reporting"]
  A -->|"Add"| I["Docker Sandbox"]
  I -->|"Include"| J["Python 3.x & 2.7"]

Loading

Grey Divider

File Changes

1. .github/workflows/codacy-analysis.yml ⚙️ Configuration changes +0/-86

Removed entire Codacy security scan workflow

.github/workflows/codacy-analysis.yml


2. .github/workflows/test-changes.yml ✨ Enhancement +24/-36

Reorganized workflow steps and enhanced reporting

.github/workflows/test-changes.yml


3. petl/test/io/test_db_server.py ✨ Enhancement +70/-34

Added environment variable-based DB connection configuration

petl/test/io/test_db_server.py


View more (6)
4. petl/test/io/test_db_create.py ✨ Enhancement +16/-15

Updated to use new DB connection argument functions

petl/test/io/test_db_create.py


5. docker-compose.yml ✨ Enhancement +119/-7

Added Python sandbox containers and network configuration

docker-compose.yml


6. requirements-database.txt Dependencies +2/-1

Added mysqlclient for Python 2.7 compatibility

requirements-database.txt


7. requirements-docs.txt Dependencies +3/-1

Added version constraints for rinohtype by Python version

requirements-docs.txt


8. requirements-formats.txt Dependencies +2/-1

Added version constraints for numexpr by Python version

requirements-formats.txt


9. tox.ini ✨ Enhancement +8/-7

Added Python 3.14 support and trace memory allocation

tox.ini


Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Jun 3, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0)

Grey Divider


Action required

1. Missing py27 MySQL headers ✓ Resolved 🐞 Bug ☼ Reliability
Description
In .github/workflows/test-changes.yml, the py2.7 matrix job installs requirements-database.txt
(which includes mysqlclient for python_version < '3') but never installs
default-libmysqlclient-dev because that step is gated on matrix.mode == 'full' while the py2.7
job sets mode: basic. This can fail the py2.7 CI run during dependency installation (or prevent
intended MySQL coverage).
Code

.github/workflows/test-changes.yml[R100-105]

Evidence
The workflow installs default-libmysqlclient-dev only when both matrix.mode and matrix.scope
are full, but the py2.7 job uses mode: basic while still installing database deps when `scope:
full. Since requirements-database.txt pulls in mysqlclient` for Python < 3, the py2.7 job can
attempt that install without the system headers; the docker-compose python2.7 sandbox explicitly
installs default-libmysqlclient-dev before installing DB requirements, indicating this header
package is expected for that environment.

.github/workflows/test-changes.yml[100-105]
.github/workflows/test-changes.yml[71-76]
.github/workflows/test-changes.yml[176-187]
requirements-database.txt[1-7]
docker-compose.yml[160-187]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The workflow installs database test dependencies (including `mysqlclient` on Python 2.7) without installing the required MySQL client development headers because `default-libmysqlclient-dev` is only installed when `matrix.mode == 'full'`.
## Issue Context
- The matrix includes a Python 2.7 job configured as `mode: basic` and `scope: full`, but DB deps are installed when `scope == full`.
- `requirements-database.txt` includes `mysqlclient` for `python_version < '3'`, so the py2.7 job will attempt to install it.
## Fix Focus Areas
- .github/workflows/test-changes.yml[100-105]
- .github/workflows/test-changes.yml[71-76]
- .github/workflows/test-changes.yml[176-187]
- requirements-database.txt[1-7]
## Suggested fix
Update the `Install additional linux tools` condition so it runs for the py2.7 DB path, e.g.:
- Change the `if:` to something like:
- `matrix.os == 'ubuntu-latest' && matrix.scope == 'full' && matrix.python < '3'`
- (or explicitly `matrix.python == '2.7'`)
Alternatively, move the `default-libmysqlclient-dev` installation into the “Install containers for remote database testing” step (guarded by the same `python < '3'` condition).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Windows CI step fails ✓ Resolved 🐞 Bug ≡ Correctness
Description
In test-changes.yml, the updated “List Installed Packages” step uses bash-style
${GITHUB_STEP_SUMMARY} expansion and printf but runs for all OSes (including windows-latest)
without shell: bash, which can cause the workflow to fail or not write the intended report on
Windows runners.
Code

.github/workflows/test-changes.yml[R188-194]

Evidence
The workflow matrix runs the job on Windows, but the modified step uses bash-only environment
variable expansion/commands without specifying a shell, so it is not portable across the matrix
OSes.

.github/workflows/test-changes.yml[42-49]
.github/workflows/test-changes.yml[185-195]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The “List Installed Packages for Throubleshooting” step appends to `"${GITHUB_STEP_SUMMARY}"` using bash syntax (`${...}`, `printf`, pipes), but the job matrix includes `windows-latest` and the step does not set `shell: bash` (nor does it guard against Windows). This can break CI runs on Windows.
## Issue Context
The job’s matrix explicitly runs on Windows, and the problematic step is unconditional.
## Fix Focus Areas
- .github/workflows/test-changes.yml[42-49]
- .github/workflows/test-changes.yml[185-196]
## Suggested fix
Pick one:
1) Add `shell: bash` to the step(s) that use bash syntax (at minimum “List Installed Packages…”, and any other step writing to `${GITHUB_STEP_SUMMARY}`), or
2) Add an OS guard (e.g., `if: runner.os != 'Windows'`) and provide a PowerShell equivalent for Windows, or
3) Replace the bash scripting with a small Python snippet that appends to `GITHUB_STEP_SUMMARY` in a cross-platform way.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Conflicting rinohtype pins ✓ Resolved 🐞 Bug ≡ Correctness
Description
requirements-docs.txt pins two different exact rinohtype versions for overlapping Python version
ranges (Python 3.7 matches both), which can cause pip to fail resolving docs dependencies. This can
block docs builds/installations for affected interpreters.
Code

requirements-docs.txt[R5-7]

Evidence
The two rinohtype entries overlap for Python 3.7.x because one range includes up to 3.7.9 and the
other starts at 3.7, making both applicable on 3.7.*.

requirements-docs.txt[5-7]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`requirements-docs.txt` includes two `rinohtype==...` lines whose environment markers overlap for Python 3.7.x, so pip can attempt to install two incompatible exact versions simultaneously.
## Issue Context
This breaks `pip install -r requirements-docs.txt` (and tox docs envs) on overlapping interpreter versions.
## Fix Focus Areas
- requirements-docs.txt[5-7]
## Suggested fix
Adjust markers so only one `rinohtype` requirement applies per interpreter.
Example (one of several valid approaches):
- `rinohtype==0.4.2 ; python_version >= '3.5' and python_version < '3.7'`
- `rinohtype==0.5.5 ; python_version >= '3.7' and python_version < '3.10'`
- `rinohtype ; python_version >= '3.10'`
If you truly need patch-level cutoffs, use `python_full_version` markers rather than `python_version`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View more (1)
4. Wrong SFTP port in-compose 🐞 Bug ≡ Correctness
Description
docker-compose.yml sets PETL_TEST_SFTP inside the python sandbox containers to use petl_sftp:2244,
but containers communicating over the compose bridge network must use the SFTP container port (22),
not the host-published port (2244). This will break SFTP remote tests when run from inside
petl-python3x/petl-python27.
Code

docker-compose.yml[R129-136]

Evidence
The compose file maps host port 2244 to container port 22 for the SFTP service, but the python
containers’ env var points at petl_sftp:2244 while on the same docker network.

docker-compose.yml[106-119]
docker-compose.yml[125-136]
docker-compose.yml[160-171]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Inside docker-compose, `PETL_TEST_SFTP` is configured to connect to `petl_sftp:2244`, but `2244` is the host-mapped port; containers should connect to the target container’s port `22` on the bridge network.
## Issue Context
The python sandbox containers (`python3x`, `python27`) are attached to `petl_network` and resolve `petl_sftp` directly, so they should use port 22.
## Fix Focus Areas
- docker-compose.yml[106-119]
- docker-compose.yml[129-136]
- docker-compose.yml[164-171]
## Suggested fix
Change the in-container SFTP URLs to use port 22 (or omit the port to default to 22), e.g.:
- `PETL_TEST_SFTP=sftp://petl:test@petl_sftp:22/public/`
Keep the host mapping `2244:22` if you still want to avoid binding host port 22.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

5. SFTP port mismatch with tox ✓ Resolved 🐞 Bug ☼ Reliability
Description
docker-compose.yml now publishes SFTP on localhost port 2244, but tox’s remote testenv still sets
PETL_TEST_SFTP without a port (defaulting to 22). Running tox -e remote/tox -e compose alongside
this compose file will point tests at the wrong port and fail SFTP remote connectivity.
Code

docker-compose.yml[R110-112]

Evidence
Compose exposes SFTP at host port 2244, while tox’s remote env var omits the port, and remote tests
iterate over PETL_TEST_ URLs to execute remote IO checks.

docker-compose.yml[106-112]
tox.ini[64-76]
petl/test/io/test_remotes.py[70-83]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Compose publishes SFTP as `2244:22`, but tox’s remote environment variable does not include `:2244`, so remote tests will try connecting to `localhost:22`.
## Issue Context
Remote tests are driven by environment variables with prefix `PETL_TEST_...`; if the URL points to the wrong port, connections fail.
## Fix Focus Areas
- docker-compose.yml[106-112]
- tox.ini[64-76]
## Suggested fix
Pick one consistent convention and apply everywhere:
- Option A (recommended): keep compose mapping `2244:22` and update tox:
- `PETL_TEST_SFTP=sftp://petl:test@localhost:2244/public/`
- update the comment command to `docker run ... -p 2244:22 ...`
- Option B: revert compose to publish `22:22` and keep tox unchanged.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


6. py314 breaks default tox ✓ Resolved 🐞 Bug ☼ Reliability
Description
tox.ini adds py314 to the default envlist but does not enable skip_missing_interpreters, so
tox can fail immediately on machines without a Python 3.14 interpreter. This degrades local
developer workflow by making the default tox run brittle.
Code

tox.ini[14]

Evidence
The envlist explicitly includes py314, and there is no [tox] skip_missing_interpreters = ...
setting present in the file.

tox.ini[13-23]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`py314` is now in the default `[tox] envlist`, but the config does not opt into skipping missing interpreters; this can cause `tox` to fail before running any tests when Python 3.14 is not installed.
## Issue Context
This affects local runs and any CI environment that doesn’t provide a 3.14 interpreter.
## Fix Focus Areas
- tox.ini[13-15]
## Suggested fix
Either:
- Add `skip_missing_interpreters = true` under `[tox]`, or
- Remove `py314` from the default envlist and run it only in dedicated CI jobs where the interpreter is guaranteed to exist.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

@coveralls

coveralls commented Jun 3, 2026

Copy link
Copy Markdown

Coverage Report for CI Build 26859421791

Coverage decreased (-0.003%) to 90.989%

Details

  • Coverage decreased (-0.003%) from the base build.
  • Patch coverage: 8 uncovered changes across 1 file (37 of 45 lines covered, 82.22%).
  • No coverage regressions found.

Uncovered Changes

File Changed Covered %
petl/test/io/test_db_server.py 38 30 78.95%
Total (2 files) 45 37 82.22%

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 14937
Covered Lines: 13591
Line Coverage: 90.99%
Coverage Strength: 0.91 hits per line

💛 - Coveralls

Comment thread requirements-docs.txt Outdated
Comment thread docker-compose.yml
@juarezr

juarezr commented Jun 3, 2026

Copy link
Copy Markdown
Member Author

/review -i

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Jun 3, 2026

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit c4a4bed

Comment thread .github/workflows/test-changes.yml Outdated
@juarezr

juarezr commented Jun 3, 2026

Copy link
Copy Markdown
Member Author

/review -i

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Jun 3, 2026

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit cefa877

Comment thread .github/workflows/test-changes.yml
@juarezr juarezr self-assigned this Jun 3, 2026
@juarezr juarezr merged commit 4ffc1c1 into petl-developers:master Jun 3, 2026
29 checks passed
@juarezr juarezr deleted the fix/ci-improve-scan-tests branch June 3, 2026 02:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Build CI CD Issues affecting github pypi readthedocs coveralls conda-forge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants