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.

Sign up for GitHub

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

Jump to bottom

Refactor test matrix: most logic at top level, call reusable workflow #348

Merged
merged 26 commits into from
Dec 20, 2024
Merged

Refactor test matrix: most logic at top level, call reusable workflow #348

merged 26 commits into from
Dec 20, 2024

Conversation

ajjackson
Copy link
Collaborator

@ajjackson ajjackson force-pushed the drop-macos-13-on-push branch from 08887dc to 1ca0ea4 Compare December 18, 2024 12:53
@ajjackson ajjackson force-pushed the drop-macos-13-on-push branch from a703d7d to 0884871 Compare December 18, 2024 13:00
@ajjackson ajjackson force-pushed the drop-macos-13-on-push branch from 9d52fc1 to 063d847 Compare December 18, 2024 13:17
@ajjackson ajjackson force-pushed the drop-macos-13-on-push branch from e905b42 to 3088f87 Compare December 18, 2024 13:20
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 18, 2024 •
edited
Loading

Test Results

    29 files  +    4      29 suites  +4   3h 33m 59s ⏱️ + 1h 6m 4s
 1 066 tests ±    0   1 060 ✅ ±    0    6 💤 ± 0  0 ❌ ±0 
20 798 runs  +4 101  20 682 ✅ +4 083  116 💤 +18  0 ❌ ±0 

Results for commit 9ef7ad4. ± Comparison against base commit 10c2639.

♻️ This comment has been updated with latest results.

@ajjackson ajjackson force-pushed the drop-macos-13-on-push branch from b3f6333 to 6560d13 Compare December 18, 2024 13:38
@ajjackson ajjackson force-pushed the drop-macos-13-on-push branch from bae1f54 to 67ff46a Compare December 18, 2024 13:50
@ajjackson
Try %0A as line break instead
0151812
@ajjackson
Rework python-versions logic
fab9371 
I can't find a simple way to pass around the multi-line inputs needed
by setup-python. This is a bit ugly but clear, at least.
@ajjackson ajjackson force-pushed the drop-macos-13-on-push branch from 12a6cdd to fab9371 Compare December 18, 2024 15:48
@ajjackson
Copy link
Collaborator Author

I'm not sure why the codacy coverage is failing to upload; I haven't changed anything related to api keys or secrets. Maybe it doesn't like the upload coming from a sub-workflow?

@ajjackson
Tweak TOX_SKIP_ENV logic: use dummy value
b3fab43 
The trouble with ${{ test && '' || output }} is that '' is "false-y"
so it returns output even if the test fails.

The return value is treated as a regex for tox env to skip, so as
long as there are no matching envs we can use any string for the "run
all tests" case.
@ajjackson
Explicitly pass codacy token as secret to reusable workflow
1e67848 
secrets are not passed to reusable workflows by default
@ajjackson
Copy link
Collaborator Author

Aha, this is because secrets are not automatically passed to reusable workflows. https://docs.github.com/en/actions/sharing-automations/reusing-workflows#passing-inputs-and-secrets-to-a-reusable-workflow

Here I pass the codacy token explicitly, but another option is to use
secrets: inherit which will pass everything.

@ajjackson
Copy link
Collaborator Author

Passing explicitly gave an error message that this one hadn't been defined in the workflow. Let's try "inherit", after all.

@ajjackson
Copy link
Collaborator Author

ajjackson commented Dec 18, 2024 •
edited
Loading

All green here: https://github.com/pace-neutrons/Euphonic/actions/runs/12397959200

now to try manual dispatch...

@ajjackson ajjackson marked this pull request as ready for review December 18, 2024 17:33
@ajjackson ajjackson requested a review from oerc0122 December 18, 2024 17:33
@ajjackson
Copy link
Collaborator Author

I hit this "random" failure on Windows:

PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'D:\a\Euphonic\Euphonic\tests_and_analysis\test\reports\coverage_1734543545.xml'

Seems like a rare collision in practice. But an unnecessary one because the Windows tests could be running without any coverage checks!

oerc0122
oerc0122 approved these changes Dec 18, 2024
View reviewed changes
Copy link
Collaborator

@oerc0122 oerc0122 left a comment

Choose a reason for hiding this comment

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

Looks good, only issue is with the vague var if.

.github/workflows/run_tests.yml Outdated Show resolved Hide resolved
@ajjackson
Copy link
Collaborator Author

Here's a random failure from coverage, having trouble with its sqlite database. I wonder if this is some kind of tox --parallel race condition that can be addressed as part of #352

https://github.com/pace-neutrons/Euphonic/actions/runs/12410831358/job/34647267908

@ajjackson
Copy link
Collaborator Author

Manual dispatch here is all green, I think this is good to merge?

https://github.com/pace-neutrons/Euphonic/actions/runs/12410831358

oerc0122
oerc0122 approved these changes Dec 19, 2024
View reviewed changes
Copy link
Collaborator

@oerc0122 oerc0122 left a comment

Choose a reason for hiding this comment

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

All looks sensible. LGTM.

To add salt though, I'd probably have gone with to_run and avoided the negation.

@ajjackson ajjackson merged commit 62be22d into master Dec 20, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oerc0122 oerc0122 oerc0122 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Drop macos-13 from run_tests.yml
2 participants
@ajjackson @oerc0122