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

Ensure that required tests always run. #1208

Closed
wants to merge 1 commit into from

Conversation

eap
Copy link
Collaborator

@eap eap commented Jul 19, 2024

This change adds "notest" variants of existing required tests. The purpose of this change is to ensure that required tests always run without forcing them to consume resources with a full build when this would be unnecessary. While the main tests use the pull_request: ↳ paths-ignore: directive to trigger the workflow, this abbreviated test uses the same file patterns but applies the inclusive paths directive.

@eap eap added the INFRA JEDI Infrastructure label Jul 19, 2024
Copy link
Collaborator

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

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

Do we really want this? I prefer to know that tests didn't run when I go back and look at the commits that were merged. This sort of fakes that tests were run when they actually weren't.

@eap
Copy link
Collaborator Author

eap commented Jul 19, 2024

@climbfuji

Yeah, that's a valid question. The problem we have is that we require these tests for merging so any PR that only includes files matching the paths-ignore filter will not be mergable without an override. GitHub doesn't give any guidance on the proper course of action here and the advice I've seen seems split between 3 choices

  1. Don't require tests that don't always need to run.

  2. Run required tests on every PR even if no affected code is changed.

  3. Make a workflow with the same name that executes against excluded paths.

I think #3 isn't as viable as I originally thought, it would probably work in a binary case but this PR shows the weakness; in case you have a test that mixes paths then the workflow run is a race condition.

So... I don't know the best path here. I'm inclined against requiring tests that may not run but due to resource limits I really don't like activating all tests for no reason. We have plenty of PRs that only update docs.

@climbfuji
Copy link
Collaborator

In this case I'd say let's do (2), because we have the runners up and running 24/7 anyway. And the rebuilds from the buildcache take only a few minutes.

@eap eap self-assigned this Jul 22, 2024
@eap
Copy link
Collaborator Author

eap commented Jul 29, 2024

moving the changes discussed here to #1210

@eap eap closed this Jul 29, 2024
@eap eap deleted the feature/always-run-named-tests branch July 29, 2024 23:09
eap added a commit that referenced this pull request Jul 30, 2024
Update the parallel cluster site config from 3.7.1 to 3.9.3. This change also updates the github test actions as detailed in #1208 ensuring that required tests run for all pull requests without consideration of file exclusion filters.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
INFRA JEDI Infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants