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

Simplify git logic in regression tests #4725

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

Conversation

kaukabrizvi
Copy link
Contributor

Resolved issues:

This PR addresses a discussion in #4701: #4701 (comment) to simplify the git logic in the regression tests.

Description of changes:

This change removes the is_older_commit and is_mainline functions of the git module to allow more flexibility in how the tests are run. Previously, two commits being compared would either have to be on the same log or one of them would have to be mainline for the auto-detection to work. This change introduces environment variables which the user can invoke to identify which commit is the "baseline" and which commit is the "altered" code.

These changes also modify the test storage scheme to still include a git commit identifier so build artifacts for a baseline commit are now stored by tests/regression_artifacts/baseline/$commit_id_$test_name. This enables the DiffProfile to access old and new commits in a simpler fashion than the previous git logic while also providing the commit id in the filename which could be useful as a reference to the change that the profile is associated with.

Additionally, the github actions workflow is updated to reflect these changes when invoking the test through CI, so that mainline sets the environment variable to baseline and the PR branch is set to altered.

Call-outs:

Since these changes modify the regression test along with the workflow, the workflow will run on this PR and fail. This is because the mainline run through the regression tests in CI will not store files in the same way that the revised approach to the tests does.

Testing:

I have run the CI workflow locally and it works as intended.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@kaukabrizvi kaukabrizvi marked this pull request as ready for review August 22, 2024 17:58
Copy link

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant