-
Notifications
You must be signed in to change notification settings - Fork 53
PTS improvements #566
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
PTS improvements #566
Conversation
…sion}` tasks. Also set REMAINING_TESTS mode for `main` branch builds on CI.
runningcode
left a comment
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 like where this is going, but I don't like the extra complexity that adding the PTS Mode adds to this already complex CI build. Is there a way we can do it without adding the extra dependency on the generate_versions_pts job?
Second thing, if we enable PTS by default, let's be sure to disable it for release jobs.
I don't see a different way to be honest, at least not a simpler one. This seems pretty straightforward to me though, If this run has
The way it is set up at the moment, the only way I see that wouldn't create a completely separate workflow for release jobs (which might also make sense to do tbh) is to use |
…red via `workflow_dispatch`)
| -PisPTSEnabled=${{ github.event.inputs.pts_enabled }} | ||
| env: | ||
| GRADLE_ENTERPRISE_ACCESS_KEY: ${{ secrets.GE_SOLUTIONS_ACCESS_TOKEN }} | ||
| ORG_GRADLE_PROJECT_isPTSEnabled: ${{ github.ref_name != 'main' }} |
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 it possible to use variable here to set the remaining tests mode inline instead of having an action to set it?
something like pts.mode: if ${{ github.ref_name == 'main' }}; then 'REMAINING_TESTS'; else 'RELEVANT_TESTS' and then pass that value?
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.
if statements can't be used in the ENV part of actions, but there are some tertiary workarounds, one of which I've applied. It does need to be duplicated on both test jobs though...
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.
Thanks for looking in to this. While there is some duplication in this solution, i prefer the simplicity of it. It also makes it easier to use as an example for customers.
* main: Use AGP 8.1.1 for main build Run tests against AGP 8.1.1 # Conflicts: # release/changes.md
| @@ -1,2 +1,2 @@ | |||
| - [NEW] Run tests against AGP 8.2.0-alpha16 | |||
| - [NEW] Run tests against AGP 8.1.1 | |||
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.
why is this showing a diff? is there a whitespace change?
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.
Yeah, I think it was without the newline at the end before. It's weird though, as I resolved it to the main branch variant of it when merging..
| -PisPTSEnabled=${{ github.event.inputs.pts_enabled }} | ||
| env: | ||
| GRADLE_ENTERPRISE_ACCESS_KEY: ${{ secrets.GE_SOLUTIONS_ACCESS_TOKEN }} | ||
| ORG_GRADLE_PROJECT_isPTSEnabled: ${{ github.ref_name != 'main' }} |
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.
Thanks for looking in to this. While there is some duplication in this solution, i prefer the simplicity of it. It also makes it easier to use as an example for customers.
runningcode
left a comment
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.
Alright as discussed and as per the manual, Let's use remaining tests mode for all pre-release cases instead of disabling PTS for those cases.
… default (also for staging builds).
runningcode
left a comment
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.
Looks great. I like the simplicity of this approach.
| run: echo "matrix=$(jq -cM '.supportedVersions | keys' src/main/resources/versions.json | sed -e 's/\./_/g' -e 's/-/_/g')" >> $GITHUB_OUTPUT | ||
| - name: debug | ||
| run: echo ${{ steps.setup-matrix.outputs.matrix }} | ||
| run: echo "matrix=${{ steps.setup-matrix.outputs.matrix }}" |
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.
just curious, why this change?
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.
Oh, it's still left from the first variant where I set the PTS mode in that job, but I still left it in as it's more verbose when looking at logs. I can remove this change if wanted.
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.
let's leave it how it originally was to simplify and reduce the changeset.
FASTPTS profile fortestAndroid${version}test tasks andSTANDARDprofile for "regular"testtask.RELEVANT_TESTSPTS mode for PRs (pre-merge) andREMAINING_TESTSformain(post-merge) branch builds.Verify Buildaction for all PRs, not just the ones targetingmain