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

CI: remove matrix redundancy between test-install and ci #6780

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

khsrali
Copy link
Contributor

@khsrali khsrali commented Mar 3, 2025

Fix for #6779

As of now, two set of tests, are redundant for no apparant reasons.
For example, see for the first commit that I made in this PR:
https://github.com/aiidateam/aiida-core/actions/runs/13631859178/job/38101273251?pr=6780#step:7:1
and
https://github.com/aiidateam/aiida-core/actions/runs/13631859180/job/38101194890?pr=6780#step:7:1

The second one has only an extra flag --cov aiida, which is irrelevant in terms of redundancy.
I don't see a reason why these tests has to be run two times.

This PR, removes that to save time on CI.

@khsrali khsrali marked this pull request as ready for review March 3, 2025 13:35
@khsrali khsrali linked an issue Mar 3, 2025 that may be closed by this pull request
@khsrali khsrali requested a review from agoscinski March 3, 2025 13:40
Copy link

codecov bot commented Mar 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.15%. Comparing base (bb5f93d) to head (9d3f1d2).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6780   +/-   ##
=======================================
  Coverage   78.15%   78.15%           
=======================================
  Files         564      564           
  Lines       42643    42643           
=======================================
  Hits        33322    33322           
  Misses       9321     9321           

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

@@ -162,7 +162,7 @@ jobs:
strategy:
fail-fast: false
matrix:
python-version: ['3.9', '3.10', '3.11', '3.12']
python-version: ['3.10', '3.11'] # '3.9' and '3.12' are tested via ci-code.yml
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that they are not exactly redundant, since in test-ci the dependencies are installed from the lockfile, while here they come from pip resolution (i.e. they will resolve to the latest version of the dependencies).

Copy link
Contributor Author

@khsrali khsrali Mar 5, 2025

Choose a reason for hiding this comment

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

Hi @danielhollas, thanks for the review, and checking here :)

I understand your point. However, I still find it somewhat redundant since it primarily checks the resolved dependencies rather than testing aiida's functionalities. In the worst case, dependency resolution could be tested separately.

If the dependencies are resolved correctly, we should technically be fine. So, running the entire test suite twice for this reason might be unnecessary.

We can discuss this further in the next aiida's meeting (tomorrow).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I am not against this change, just wanted to point it out.

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.

CI: duplicated tests for 3.9 and 3.12 with psql database backend
2 participants