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

Disable PTS from local and enable it for PRs #5826

Merged
merged 4 commits into from
Feb 27, 2023

Conversation

BraisGabin
Copy link
Member

The github yaml configuration make this a bit ugly.

This was discussed with the gradle folks on how to improve the usage of PTS.

PTS on local doesn't provide us too much benefits because it breaks the incremental compilariont (UP-TO-DATE check).

The idea is that we make our PR checks faster now that we have a trained PTS but we run all our tests once it's merged to main to ensure that nothing was broken.

@BraisGabin
Copy link
Member Author

@clayburn or @runningcode could you take a look to this one?

@codecov
Copy link

codecov bot commented Feb 24, 2023

Codecov Report

Merging #5826 (a24a2aa) into main (2ae921a) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #5826      +/-   ##
============================================
- Coverage     84.57%   84.56%   -0.01%     
+ Complexity     3787     3785       -2     
============================================
  Files           546      546              
  Lines         12936    12935       -1     
  Branches       2273     2273              
============================================
- Hits          10940    10939       -1     
  Misses          863      863              
  Partials       1133     1133              
Impacted Files Coverage Δ
...itlab/arturbosch/detekt/rules/style/MagicNumber.kt 91.01% <0.00%> (-0.20%) ⬇️
.../io/gitlab/arturbosch/detekt/rules/GuardClauses.kt 13.33% <0.00%> (+6.19%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@BraisGabin BraisGabin added the housekeeping Marker for housekeeping tasks and refactorings label Feb 24, 2023
Copy link
Contributor

@clayburn clayburn left a comment

Choose a reason for hiding this comment

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

I'm only mildly familiar with the GitHub Actions part. If github.event_name == 'push' && github.ref == 'refs/heads/main' executes on main after pull requests are merged to main, then this looks good to me! The intent of this looks correct.

build.gradle.kts Outdated Show resolved Hide resolved
@BraisGabin BraisGabin force-pushed the imptove-pts-configuration branch from 01eba25 to c359c38 Compare February 26, 2023 19:37
build.gradle.kts Outdated Show resolved Hide resolved
.github/workflows/pre-merge.yaml Outdated Show resolved Hide resolved
BraisGabin and others added 2 commits February 26, 2023 21:59
Co-authored-by: Róbert Papp <papp.robert.s@gmail.com>
Copy link
Member

@TWiStErRob TWiStErRob left a comment

Choose a reason for hiding this comment

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

Looks minimally correct to me :)

@BraisGabin
Copy link
Member Author

I would like to have more eyes on this to ensure that we are ok with the change that this implies. The code is really simple now but I want to be sure that we agree with this. Don't run ALL the tests on each PR is a big deal.

Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

I would like to have more eyes on this

+1 on my end for this

@BraisGabin BraisGabin merged commit 77f6c60 into main Feb 27, 2023
@BraisGabin BraisGabin deleted the imptove-pts-configuration branch February 27, 2023 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build ci housekeeping Marker for housekeeping tasks and refactorings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants