-
Notifications
You must be signed in to change notification settings - Fork 10
fix: fix integration test automated run #472
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: fix integration test automated run #472
Conversation
Signed-off-by: Pablo Garay <[email protected]>
Signed-off-by: Pablo Garay <[email protected]>
.github/workflows/cicd-main.yml
Outdated
| github.ref == 'refs/heads/main' | ||
| || github.event_name == 'workflow_dispatch' | ||
| || needs.pre-flight.outputs.is_ci_workload == 'true' | ||
| || needs.pre-flight.outputs.force_run_all == 'true' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_ci_workload already accounts for whether it is on main branch or not
https://github.com/NVIDIA-NeMo/FW-CI-templates/blob/845ca1e302e9592b067c7696e372fa63fd1b8b6f/.github/workflows/_cicd_preflight.yml#L116
Is force_run_all even a part of the expected workflow for these tests? If this only runs in the "main" environment, which I think is just main branch? Then there will never be a force-run-all label.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think is just main branch
@chtruong814 I asked Pablo to also allow manual run for any branch. We basically want to run CI without this test for PRs, but have an option to trigger it if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw
|| needs.pre-flight.outputs.force_run_all == 'true'
is consistently used across all the file (e.g. look above lines) & yes it wouldnt have effect though i would suggest we make that change consistently (for consistency) in follow up PR then (removing all other occurrences in other places) - if thats wanted to be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I guess I'm confused. In the example you shared above, the integration test is running but fails due to the Github environment configuration.
https://github.com/NVIDIA-NeMo/Evaluator/actions/runs/19499650277/job/55810558564
So do we want to run this test successfully only on main branch or also allow other branches?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pablo-garay where are you expecting this branch variable to be used?
https://github.com/NVIDIA-NeMo/Evaluator/blob/main/.github/workflows/cicd-main.yml#L18
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I stand corrected, my bad. It's been extra hectic busy so didnt get proper time for more thorough testing
"fails due to the Github environment configuration." -> needed a fix, updated now
So do we want to run this test successfully only on main branch or also allow other branches?
where are you expecting this branch variable to be used?
IIUC your question, These are the input params
Signed-off-by: Pablo Garay <[email protected]>
- Add condition to run integration tests when pushing to main branch - Integration tests now run on: scheduled cron, manual triggers, and main branch pushes - PRs continue to skip integration tests as intended
.github/workflows/cicd-main.yml
Outdated
|
|
||
| cicd-integration-tests: | ||
| needs: | ||
| - pre-flight |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't need pre-flight anymore right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true ; this was remnant of previous approach
Signed-off-by: Pablo Garay <[email protected]>
|
/ok to test dc7dc8e |


Integration tests triggering:
Plan : to skip them for PRs, but we wanted to run them on main and I'd like to be able to trigger them manually too
This would now satisfies requirements:
Req 1a: PRs with /ok to test xyz will SKIP integration tests (they use push events to pull-request/* branches, not schedule or workflow_dispatch)
Req 1b: Merges to main won't automatically run integration tests (also push events), but scheduled runs will catch them
Req 2: Manual workflow_dispatch triggers from any branch will run integration tests
Req 3: No more environment protection errors when triggering from feature branches
The key insight is that by checking github.event_name directly instead of relying on is_ci_workload, we:
Fixed integration tests triggering logic:
Tested:
For:
https://github.com/NVIDIA-NeMo/Evaluator/actions/runs/19550694641
another test: https://github.com/NVIDIA-NeMo/Evaluator/actions/runs/19552356088/job/55986938425