Skip to content

Conversation

michaelbrownuc
Copy link
Collaborator

@michaelbrownuc michaelbrownuc commented Aug 14, 2025

closes #287

I've fixed up the CI organization into unit tests that run on every commit and component level / system level integration tests. The integration tests run nightly or when labelled.

@CLAassistant
Copy link

CLAassistant commented Aug 14, 2025

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ michaelbrownuc
❌ Michael D. Brown


Michael D. Brown seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

This comment was marked as outdated.

@dguido
Copy link
Member

dguido commented Aug 15, 2025

Hey Mike, here's a recap of what I was trying to work on that Riccardo rolled back in PR #271. There is a lot of existing code there that might be useful to cherry pick. Here were my objectives and some of the reasoning behind how I modified the CI (obviously Claude Code summarized this). You could probably give each of these to Claude Code one at a time and it would be able to follow all these steps with ease.

Suggested Improvements for PR #288

Excellent work on reorganizing the CI workflows! Once PR #285 is merged, the mypy type checking issues (including the fuzzer component) will no longer be a blocker, so this reorganization comes at a perfect time. Here are some opportunities to further optimize the workflow efficiency:

1. Reduce Duplication in comp-integration.yml Dependency Installation

The new workflow has three separate dependency installation blocks that could be consolidated using matrix properties:

matrix:
  include:
    - component: common
      coverage_module: buttercup.common
      python: "3.12"
      needs_codequery: false
    - component: program-model
      coverage_module: buttercup.program_model
      python: "3.12"
      needs_codequery: true
      needs_wasm: false
    - component: seed-gen
      coverage_module: buttercup.seed_gen
      python: "3.12"
      needs_codequery: true
      needs_wasm: true

Then simplify to a single installation step:

- name: Install dependencies
  run: |
    sudo apt-get update
    sudo apt-get install -y ripgrep
    if [[ "${{ matrix.needs_codequery }}" == "true" ]]; then
      sudo apt-get install -y codequery
      make install-cscope
    fi

- name: Download Wasm runtime
  if: matrix.needs_wasm
  run: wget https://github.com/vmware-labs/webassembly-language-runtimes/releases/download/python%2F3.12.0%2B20231211-040d5a6/python-3.12.0.wasm
  working-directory: ${{ matrix.component }}

Why this matters: Reduces 4 conditional installation blocks to 1-2, making the workflow more maintainable and less prone to drift.

2. Consolidate Duplicate Test Jobs

The test and test-integration jobs in comp-integration.yml are nearly identical (~170 lines each). Consider using a reusable workflow or parameterizing:

jobs:
  test:
    strategy:
      matrix:
        include:
          - component: common
            coverage_module: buttercup.common
            test_type: unit
          # ... other components
          
  test-integration:
    uses: ./.github/workflows/comp-integration.yml
    with:
      matrix_override:
        test_type: integration
        pytest_args: "--runintegration"

Or at minimum, use a conditional pytest command:

- name: Run tests
  run: |
    PYTEST_ARGS="-svv --junit-xml=${{ matrix.test_type }}-results.xml"
    [[ "${{ matrix.test_type }}" == "integration" ]] && PYTEST_ARGS="$PYTEST_ARGS --runintegration"
    uv run --frozen pytest $PYTEST_ARGS \
      --cov=${{ matrix.coverage_module }} \
      --cov-report=xml --cov-report=html

Why this matters: Eliminates ~170 lines of duplicate YAML, ensures consistency between test types, and makes future changes easier to apply uniformly.

3. Optimize Matrix Strategy with Early Termination

Since PR #285 temporarily ignores mypy issues, we can now use fail-fast more intelligently:

strategy:
  fail-fast: ${{ github.event_name == 'pull_request' }}  # Fail fast on PRs, run all on main
  matrix:
    # ...

This speeds up PR feedback while ensuring comprehensive testing on main branch.

4. YAML Style Consistency

The workflows still have mixed quote styles. A quick standardization would improve readability:

# Current (mixed):
- 'common/**'
- "fuzzer/**"

# Standardized:
- "common/**"
- "fuzzer/**"

5. Consider Workflow Composition Strategy

Instead of moving all integration tests to a separate file, consider:

  • Keep fast unit tests in tests.yml
  • Move only the heavy integration tests to comp-integration.yml
  • Use workflow_call to share common job templates

This would maintain quick feedback for PRs while organizing longer-running tests appropriately.

Additional note: With the type checking now not blocking due to PR #285, the lint workflow can be further simplified to remove any lingering special handling or comments about fuzzer issues.

These changes would reduce the workflow maintenance burden from ~720 lines to ~400 lines while improving clarity and consistency. The key principle from PR #262's history is that unified, parameterized approaches beat duplicate job definitions.

@michaelbrownuc
Copy link
Collaborator Author

Hey Mike, here's a recap of what I was trying to work on that Riccardo rolled back in PR #271. There is a lot of existing code there that might be useful to cherry pick. Here were my objectives and some of the reasoning behind how I modified the CI (obviously Claude Code summarized this). You could probably give each of these to Claude Code one at a time and it would be able to follow all these steps with ease.

Suggested Improvements for PR #288

Excellent work on reorganizing the CI workflows! Once PR #285 is merged, the mypy type checking issues (including the fuzzer component) will no longer be a blocker, so this reorganization comes at a perfect time. Here are some opportunities to further optimize the workflow efficiency:

1. Reduce Duplication in comp-integration.yml Dependency Installation

The new workflow has three separate dependency installation blocks that could be consolidated using matrix properties:

matrix:
  include:
    - component: common
      coverage_module: buttercup.common
      python: "3.12"
      needs_codequery: false
    - component: program-model
      coverage_module: buttercup.program_model
      python: "3.12"
      needs_codequery: true
      needs_wasm: false
    - component: seed-gen
      coverage_module: buttercup.seed_gen
      python: "3.12"
      needs_codequery: true
      needs_wasm: true

Then simplify to a single installation step:

- name: Install dependencies
  run: |
    sudo apt-get update
    sudo apt-get install -y ripgrep
    if [[ "${{ matrix.needs_codequery }}" == "true" ]]; then
      sudo apt-get install -y codequery
      make install-cscope
    fi

- name: Download Wasm runtime
  if: matrix.needs_wasm
  run: wget https://github.com/vmware-labs/webassembly-language-runtimes/releases/download/python%2F3.12.0%2B20231211-040d5a6/python-3.12.0.wasm
  working-directory: ${{ matrix.component }}

Why this matters: Reduces 4 conditional installation blocks to 1-2, making the workflow more maintainable and less prone to drift.

2. Consolidate Duplicate Test Jobs

The test and test-integration jobs in comp-integration.yml are nearly identical (~170 lines each). Consider using a reusable workflow or parameterizing:

jobs:
  test:
    strategy:
      matrix:
        include:
          - component: common
            coverage_module: buttercup.common
            test_type: unit
          # ... other components
          
  test-integration:
    uses: ./.github/workflows/comp-integration.yml
    with:
      matrix_override:
        test_type: integration
        pytest_args: "--runintegration"

Or at minimum, use a conditional pytest command:

- name: Run tests
  run: |
    PYTEST_ARGS="-svv --junit-xml=${{ matrix.test_type }}-results.xml"
    [[ "${{ matrix.test_type }}" == "integration" ]] && PYTEST_ARGS="$PYTEST_ARGS --runintegration"
    uv run --frozen pytest $PYTEST_ARGS \
      --cov=${{ matrix.coverage_module }} \
      --cov-report=xml --cov-report=html

Why this matters: Eliminates ~170 lines of duplicate YAML, ensures consistency between test types, and makes future changes easier to apply uniformly.

3. Optimize Matrix Strategy with Early Termination

Since PR #285 temporarily ignores mypy issues, we can now use fail-fast more intelligently:

strategy:
  fail-fast: ${{ github.event_name == 'pull_request' }}  # Fail fast on PRs, run all on main
  matrix:
    # ...

This speeds up PR feedback while ensuring comprehensive testing on main branch.

4. YAML Style Consistency

The workflows still have mixed quote styles. A quick standardization would improve readability:

# Current (mixed):
- 'common/**'
- "fuzzer/**"

# Standardized:
- "common/**"
- "fuzzer/**"

5. Consider Workflow Composition Strategy

Instead of moving all integration tests to a separate file, consider:

  • Keep fast unit tests in tests.yml
  • Move only the heavy integration tests to comp-integration.yml
  • Use workflow_call to share common job templates

This would maintain quick feedback for PRs while organizing longer-running tests appropriately.

Additional note: With the type checking now not blocking due to PR #285, the lint workflow can be further simplified to remove any lingering special handling or comments about fuzzer issues.

These changes would reduce the workflow maintenance burden from ~720 lines to ~400 lines while improving clarity and consistency. The key principle from PR #262's history is that unified, parameterized approaches beat duplicate job definitions.

Thanks I'll take a look!

Comment on lines 37 to 200
restore-keys: |
${{ runner.os }}-uv-integration-${{ matrix.component }}-
${{ runner.os }}-uv-

- name: Download Wasm runtime
if: matrix.component == 'seed-gen' && steps.should_test.outputs.test != 'false'
run: wget https://github.com/vmware-labs/webassembly-language-runtimes/releases/download/python%2F3.12.0%2B20231211-040d5a6/python-3.12.0.wasm
working-directory: seed-gen

- name: Install integration test dependencies
if: steps.should_test.outputs.test != 'false'
run: |
sudo apt-get update
sudo apt-get install -y codequery ripgrep
make install-cscope

- name: Prepare environment
if: steps.should_test.outputs.test != 'false'
run: |
export DEBIAN_FRONTEND=noninteractive
sudo apt-get update
sudo mkdir -p /crs_scratch
sudo chmod -R 777 /crs_scratch

- name: Setup ${{ matrix.component }} component
if: steps.should_test.outputs.test != 'false'
run: |
uv sync --all-extras --frozen
uv pip install --isolated pytest-html>=4.1.1 pytest-cov>=6.0.0
working-directory: ${{ matrix.component }}

- name: Run program-model libpng integration test
if: matrix.component == 'program-model' && steps.should_test.outputs.test != 'false'
run: |
uv run --frozen pytest -svv --runintegration tests/c/test_libpng.py \
--junit-xml=integration-test-results.xml \
--html=integration-test-report.html \
--self-contained-html \
--cov=${{ matrix.coverage_module }} \
--cov-report=xml:integration-coverage.xml \
--cov-report=html:integration-htmlcov \
--cov-report=term
env:
PYTHON_WASM_BUILD_PATH: "python-3.12.0.wasm"
working-directory: ${{ matrix.component }}
timeout-minutes: 30

- name: Run integration tests on ${{ matrix.component }}
if: matrix.component != 'program-model' && steps.should_test.outputs.test != 'false'
run: |
uv run --frozen pytest -svv --runintegration \
--junit-xml=integration-test-results.xml \
--html=integration-test-report.html \
--self-contained-html \
--cov=${{ matrix.coverage_module }} \
--cov-report=xml:integration-coverage.xml \
--cov-report=html:integration-htmlcov \
--cov-report=term
env:
PYTHON_WASM_BUILD_PATH: "python-3.12.0.wasm"
working-directory: ${{ matrix.component }}
timeout-minutes: 30

- name: Generate integration test summary
if: always() && steps.should_test.outputs.test != 'false'
run: |
echo "### Integration Test Results: ${{ matrix.component }}" >> $GITHUB_STEP_SUMMARY
echo "" >> $GITHUB_STEP_SUMMARY
if [ -f ${{ matrix.component }}/integration-test-results.xml ]; then
python -c "
import xml.etree.ElementTree as ET
tree = ET.parse('${{ matrix.component }}/integration-test-results.xml')
root = tree.getroot()
tests = root.get('tests', '0')
failures = root.get('failures', '0')
errors = root.get('errors', '0')
skipped = root.get('skipped', '0')
time = root.get('time', '0')
print(f'- **Total Tests**: {tests}')
print(f'- **Passed**: {int(tests) - int(failures) - int(errors) - int(skipped)}')
print(f'- **Failed**: {failures}')
print(f'- **Errors**: {errors}')
print(f'- **Skipped**: {skipped}')
print(f'- **Duration**: {float(time):.2f}s')
" >> $GITHUB_STEP_SUMMARY
else
echo "No integration test results found" >> $GITHUB_STEP_SUMMARY
fi

- name: Upload integration test results
if: always() && steps.should_test.outputs.test != 'false'
uses: actions/upload-artifact@v4
with:
name: integration-test-results-${{ matrix.component }}-py${{ matrix.python }}
path: |
${{ matrix.component }}/integration-test-results.xml
${{ matrix.component }}/integration-test-report.html
${{ matrix.component }}/integration-coverage.xml
${{ matrix.component }}/integration-htmlcov/
retention-days: 30

Check warning

Code scanning / CodeQL

Workflow does not contain permissions Medium

Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {contents: read}
@michaelbrownuc
Copy link
Collaborator Author

michaelbrownuc commented Aug 26, 2025

One of the final two tests that are failing is due to the commit 39d4001 not being available in our mirror of oss-fuzz-aixcc. In the source repo this commit is orphaned and my attempts to bring it over have failed. For now I will disable these tests to get this PR merged. We can re-enable them down the line if we want / can fix the root issue.

The other needs diagnosing but can be done separately from this PR. I've made an issue for it #324

@michaelbrownuc michaelbrownuc marked this pull request as ready for review August 26, 2025 20:36
@michaelbrownuc michaelbrownuc added the integration-tests Component Integration Tests (~30 min). Test interactions with Redis, CodeQuery, file systems, etc. label Aug 26, 2025
Copy link
Collaborator

@evandowning evandowning left a comment

Choose a reason for hiding this comment

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

LGTM

@michaelbrownuc michaelbrownuc merged commit 2284fc1 into main Aug 27, 2025
22 checks passed
@michaelbrownuc michaelbrownuc deleted the ci/ci-fixes branch August 27, 2025 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full-integration integration-tests Component Integration Tests (~30 min). Test interactions with Redis, CodeQuery, file systems, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Separate CI workflows for unit tests and component integration tests
5 participants