Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: CI #190

Merged
merged 15 commits into from
Oct 2, 2024
Merged

Fix: CI #190

merged 15 commits into from
Oct 2, 2024

Conversation

Ckk3
Copy link
Contributor

@Ckk3 Ckk3 commented Sep 29, 2024

Description

Fix CI tests by updating GitHub Actions workflows.
One key point to note: the Python 3.8 and 3.9 tests are consistently failing, and this is not related to the GitHub Actions workflow. I verified that we had previously identified this issue, as referenced in a comment by Philaeux.

The issue was resolved by adding the name parameter to the BigInt scalar definition. This prevents an error from being raised when strawberry.scalar is called and __name__ is searched for inside a Union type in this part of the Strawberry code.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

Summary by Sourcery

Fix CI tests by updating GitHub Actions workflows to use the latest versions of actions and dependencies, including actions/checkout, actions/setup-python, and codecov/codecov-action. Replace pipx with pip for dependency installation and adjust Python version specifications and caching strategies.

Bug Fixes:

  • Fix CI tests by updating GitHub Actions workflows to use the latest versions of actions and dependencies.

CI:

  • Update GitHub Actions workflows to use actions/checkout@v4 and actions/setup-python@v5.
  • Replace pipx with pip for installing poetry, nox, and nox-poetry.
  • Update codecov/codecov-action to version v4.
  • Adjust Python version specifications and caching strategies in CI workflows.

@Ckk3 Ckk3 self-assigned this Sep 29, 2024
Copy link

codspeed-hq bot commented Sep 29, 2024

CodSpeed Performance Report

Merging #190 will not alter performance

Comparing Ckk3:fix-ci-tests (95b788d) with main (f9c09a1)

Summary

✅ 1 untouched benchmarks

@codecov-commenter
Copy link

codecov-commenter commented Oct 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.32%. Comparing base (f9c09a1) to head (95b788d).

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #190       +/-   ##
===========================================
+ Coverage   43.34%   85.32%   +41.98%     
===========================================
  Files          16       16               
  Lines        1615     1615               
  Branches      226      320       +94     
===========================================
+ Hits          700     1378      +678     
+ Misses        886      183      -703     
- Partials       29       54       +25     

@Ckk3 Ckk3 marked this pull request as ready for review October 2, 2024 03:34
Copy link
Contributor

sourcery-ai bot commented Oct 2, 2024

Reviewer's Guide by Sourcery

This pull request focuses on fixing CI tests on GitHub Actions. The changes primarily involve updating the CI workflow configuration, upgrading dependencies, and making minor adjustments to improve the testing process.

Architecture diagram for updated CI workflow

graph TD;
    A[GitHub Actions] -->|uses| B[actions/checkout@v4];
    B -->|runs| C[setup-python@v5];
    C -->|installs| D[poetry, nox, nox-poetry, coverage];
    D -->|runs| E[nox -r -t tests];
    E -->|generates| F[coverage xml];
    F -->|uploads| G[codecov/codecov-action@v4];
    H[ikalnytskyi/action-setup-postgres@v4] -->|sets up| I[PostgreSQL];
    J[CodSpeedHQ/action@v2] -->|runs| K[benchmarks];
    L[actions/cache@v4] -->|caches| M[~/.cache, ~/.nox, .nox];
Loading

File-Level Changes

Change Details Files
Update GitHub Actions workflow configuration
  • Upgrade actions/checkout from v3 to v4
  • Replace wntrblm/nox action with direct installation of nox and poetry
  • Update Python setup to use actions/setup-python@v5
  • Add Python 3.13-dev to the test matrix
  • Simplify caching strategy
  • Update codecov/codecov-action from v3 to v4
  • Update CodSpeedHQ/action from v1 to v2
  • Upgrade actions/cache from v3 to v4
  • Add uv installation in the lint job
.github/workflows/test.yml
Modify Python version and dependency management
  • Change Python version from 3.12.0-rc.2 to 3.12 in benchmark job
  • Remove explicit pip upgrade step
  • Simplify poetry environment setup
.github/workflows/test.yml
Adjust BigInt scalar definition
  • Add 'name' parameter to BigInt scalar definition
src/strawberry_sqlalchemy_mapper/scalars.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@botberry
Copy link
Member

botberry commented Oct 2, 2024

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


Resolved an issue with the BigInt scalar definition, ensuring compatibility with Python 3.8 and 3.9. The missing name parameter was added to prevent runtime errors.
Fixed failing CI tests by updating the GitHub Actions workflow to improve test stability.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @Ckk3 - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Consider updating the PR description to mention all changes, including the modification to the BigInt scalar in scalars.py.
  • The removal of the Python version matrix in the lint job might reduce cross-version linting coverage. Consider keeping multi-version linting or explain the rationale for this change.
Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟡 Testing: 1 issue found
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines 103 to -141
lint:
name: ✨ Lint
runs-on: ubuntu-latest
strategy:
fail-fast: false

steps:
- uses: actions/checkout@v3
- uses: wntrblm/nox@main
- uses: actions/checkout@v4
- uses: actions/setup-python@v5
with:
python-versions: "3.8, 3.9, 3.10, 3.11, 3.12"
python-version: |
3.8
3.9
3.10
3.11
3.12

- name: Pip and nox cache
id: cache
uses: actions/cache@v3
uses: actions/cache@v4
with:
path: |
~/.cache
~/.nox
.nox
key: ${{ runner.os }}-nox-lint-${{ matrix.session.session }}-${{
key:
${{ runner.os }}-nox-lint-${{ env.pythonLocation }}-${{
hashFiles('**/poetry.lock') }}-${{ hashFiles('**/noxfile.py') }}
restore-keys: |
${{ runner.os }}-lint-nox-${{ matrix.session.session }}-
${{ runner.os }}-lint-nox-
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (performance): Reconsider removing the caching step in the lint job

Removing the caching step for pip and nox in the lint job might slow down the CI process. Consider keeping a form of caching to improve performance, especially for frequently run jobs like linting.

      - uses: actions/checkout@v4
      - uses: actions/setup-python@v5
        with:
          python-version: |
            3.8
            3.9
            3.10
            3.11
            3.12
      - uses: actions/cache@v4
        with:
          path: ~/.cache/pip
          key: ${{ runner.os }}-pip-${{ hashFiles('**/poetry.lock') }}
          restore-keys: |
            ${{ runner.os }}-pip-

@Ckk3
Copy link
Contributor Author

Ckk3 commented Oct 2, 2024

Ready for Review!

I’ve updated the CI and incorporated a lot from the Strawberry repository.

One key point to note: the Python 3.8 and 3.9 tests are consistently failing, and this is not related to the GitHub Actions workflow. I verified that we had previously identified this issue, as referenced in a comment by Philaeux.

The issue was resolved by adding the name parameter to the BigInt scalar definition. This prevents an error from being raised when strawberry.scalar is called and __name__ is searched for inside a Union type in this part of the Strawberry code.

Copy link
Member

@patrick91 patrick91 left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

@patrick91 patrick91 merged commit 5b09eee into strawberry-graphql:main Oct 2, 2024
19 checks passed
@Ckk3 Ckk3 deleted the fix-ci-tests branch October 2, 2024 13:36
yovel-clutch added a commit to clutchsecurity/strawberry-sqlalchemy that referenced this pull request Oct 9, 2024
* Fix: CI (strawberry-graphql#190)

* creatign a new PR just to test actions

* uses default github action from strawberry repo

* making a test wrong to see if gets a error

* make test correct again

* take back poetry command

* another test

* test with coverage cml

* add coverage package

* change codecove version

* change order

* ops

* trying wiht no async to see whats wrong with xdist

* bring back async

* fix: BigInt raising errors

* add release md

* Release 🍓 0.4.4

---------

Co-authored-by: Luis Gustavo <[email protected]>
Co-authored-by: Botberry <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants