Skip to content

CI: isolate network tests from required suite - #752

Merged
d-chambers merged 2 commits into
devfrom
ci-isolate-network-tests
Jul 14, 2026
Merged

CI: isolate network tests from required suite#752
d-chambers merged 2 commits into
devfrom
ci-isolate-network-tests

Conversation

@d-chambers

@d-chambers d-chambers commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Description

The localhost HTTP/fsspec tests are marked network and can intermittently hang while h5py probes remote HDF5 metadata. Because they currently run inside the required test matrix, those transport-level flakes can block unrelated changes.

This PR separates that coverage from the required suite:

  • the default test runner now selects -m "not network" and retains coverage reporting;
  • an explicit network runner mode selects all tests carrying the marker;
  • a report-only Linux/macOS job runs the network tests on the shared default Python version; and
  • the network matrix has fail-fast disabled and is allowed to fail, so both platforms report without blocking the workflow;
  • successful network jobs upload a separate Codecov flag; and
  • the network flag carries coverage forward when a report-only run flakes, preserving project coverage without making that run gating.

This is a CI-policy change only. The remote-HDF5 implementation and its existing tests are unchanged.

Validation:

  • ./.github/test_code.sh network: 100 passed initially; coverage-enabled revalidation passed 99 with 1 timeout skip.
  • ./.github/test_code.sh: 7229 passed, 87 skipped, 100 deselected, 3 xfailed.
  • targeted pre-commit hooks: YAML and GitHub Actions lint passed.
  • Codecov configuration validation: valid.

Changelog

none

Checklist

I have (if applicable):

  • referenced the GitHub issue this PR closes.
  • documented the new feature with docstrings and/or appropriate doc page.
  • included tests. See testing guidelines.
  • added the "ready_for_review" tag once the PR is ready to be reviewed.

Summary by CodeRabbit

  • Tests

    • Added network test coverage on Ubuntu and macOS (non-blocking / report-only).
    • Improved test argument handling to preserve command-line options reliably.
    • Network test failures now appear in the workflow summary without blocking overall validation (when applicable).
  • Chores

    • Exposed the configured default Python version as a reusable workflow output.
    • Updated code coverage carryforward rules to apply separately by flag group (unit vs network).

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@d-chambers d-chambers added the ready_for_review PR is ready for review label Jul 12, 2026
@coderabbitai

coderabbitai Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

CI network test integration

Layer / File(s) Summary
Expose the default Python version
.github/actions/load-shared-vars/action.yml, .github/workflows/runtests.yml
The shared action emits python-default, and the setup job exposes it as a workflow output.
Run report-only network tests
.github/test_code.sh, .github/workflows/runtests.yml
Pytest arguments are passed as a Bash array, and network tests run on Ubuntu and macOS with non-blocking failure reporting.
Configure coverage carryforward
codecov.yml
Unit-test coverage disables carryforward, while network coverage enables it.

Possibly related PRs

  • DASDAE/dascore#591: Both changes handle the shared default Python version through the shared action.
  • DASDAE/dascore#593: Both changes update shared action output and environment handling.

Suggested labels: CI

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main CI change: isolating network tests from the required suite.
Description check ✅ Passed The description matches the template with a clear summary and checklist, and it includes validation details.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ci-isolate-network-tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot added the CI continuous integration label Jul 12, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
.github/workflows/runtests.yml (1)

142-145: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Consider setting persist-credentials: false on the checkout step.

The actions/checkout@v4 action persists the GITHUB_TOKEN in the local git config by default. Since this job only runs tests and performs no git pushes or authenticated registry operations, disabling credential persistence reduces the risk of token leakage through artifacts or debug outputs.

🔒️ Suggested change
       - uses: actions/checkout@v4
         with:
           fetch-tags: 'true'
           fetch-depth: '0'
+          persist-credentials: false
🤖 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 @.github/workflows/runtests.yml around lines 142 - 145, Update the
actions/checkout@v4 step to set persist-credentials to false alongside the
existing fetch settings, while preserving fetch-tags and fetch-depth behavior.

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.

Nitpick comments:
In @.github/workflows/runtests.yml:
- Around line 142-145: Update the actions/checkout@v4 step to set
persist-credentials to false alongside the existing fetch settings, while
preserving fetch-tags and fetch-depth behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bd75b1db-7537-42ca-bbac-0bc9ea9a3e3d

📥 Commits

Reviewing files that changed from the base of the PR and between 1a200d8 and 764440e.

📒 Files selected for processing (3)
  • .github/actions/load-shared-vars/action.yml
  • .github/test_code.sh
  • .github/workflows/runtests.yml

@codecov

codecov Bot commented Jul 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (1a200d8) to head (be9fbfd).

Additional details and impacted files
@@            Coverage Diff            @@
##               dev      #752   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          152       152           
  Lines        14929     14929           
=========================================
  Hits         14929     14929           
Flag Coverage Δ
network 51.63% <ø> (?)
unittests 99.97% <ø> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@d-chambers
d-chambers merged commit a79e8e5 into dev Jul 14, 2026
27 checks passed
@d-chambers
d-chambers deleted the ci-isolate-network-tests branch July 14, 2026 06:40
@d-chambers d-chambers removed the ready_for_review PR is ready for review label Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI continuous integration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant