DX: Improve QA process #7366
DX: Improve QA process #7366
Conversation
Wirone
force-pushed
the
codito/improve-qa-process
branch
2 times, most recently
from
390c5d9
to
c58e279
Compare
Wirone
force-pushed
the
codito/improve-qa-process
branch
from
699eeb6
to
655eba1
Compare
Wirone
force-pushed
the
codito/improve-qa-process
branch
from
e9b907c
to
f91a629
Compare
| job-description: 'with lowest deps' | ||
| job-description: 'tests with Symfony ^5' | ||
| run-tests: 'yes' | ||
| execute-flex-with-symfony-version: '^5' # explicit check for Symfony 5.x compatibility |
There was a problem hiding this comment.
likely "7.4 tests with Sf^5" and "7.4 tests with lowest-deps" can be merged
yes, test will be less atomic. one will open log and see why it failed
There was a problem hiding this comment.
I am a bit struggling now, and wondering about "split of responsibilities" vs "run the few tasks on same machine".
IE - PHP 7.4, we have:
- 7.4 test Sf 5
- 7.4 tests with lowest deps (missing now)
- 7.4 fixer with lowest deps
- 7.4 fixer with phpdoc2code
- 7.4 fixer with Sf ^5
First proposal:
- 7.4 tests with lowest deps + Sf 5 explicitly
- 7.4 fixer with lowest deps + Sf 5 explicitly + phpdoc2code
2nd proposal (for discussion):
approx 15s is for each job to build PHP and install composer deps. Why not having single - 7.4 with lowest deps + Sf 5 explicitly + fixer phpdoc2code, fixer, tests
?
There was a problem hiding this comment.
Because it's cumbersome to look inside the job every time to see what actually failed. When you have complete list of checks in the PR, you clearly see on the list what succeeded and what failed. Personally I prefer wider list of atomic checks than condensed jobs with multiple checks inside.
There was a problem hiding this comment.
there are 20 jobs. we could reduce to ~7job, so 13x15s ~~ 3mins :D
honestly, see no value of keeping "lowest deps" and "sf5" separately
There was a problem hiding this comment.
The actual value is that tests (~6min) are run before Fixer (~1min), so you effectively get feedback about CS violations few minutes later than you could. There's no point in waiting few minutes for something that you could get earlier and fix it even before tests are finished.
There was a problem hiding this comment.
we stop all jobs when any of them fails, right? eg we stop PHPUnit in-progress task, when fixer check will fail?
if so, I'm OK with that proposal to run them in parallel, as separated jobs
There was a problem hiding this comment.
No, we don't stop jobs when any of them fails, but why should we? The point of this is exactly the opposite: to know that tests are passing, even if there are CS violations, and vice versa. Currently we wait with CS check for tests to complete, which makes the job longer, but also is not developer friendly - imagine scenario:
- developer pushes branch
- tests are failing
- developer fixes tests and pushes branch
- CS check is failing
- developer must fix the branch again and push it again, which means 3 pipeline runs (can be 2).
This feedback cycle is suboptimal, with my approach you have explicit info about both: tests and CS check. Seeing both failed developer can fix both, with one feedback cycle.
There was a problem hiding this comment.
I'm actually eager to have a cascatde and terminate jobs ASAP, for any CI. if we go, let say, SCA->Utests, I do not need to run tests if SCA is failing. Dev need to come back to local env and do fixes. and they should not push code if they didn't test it locally.
if we run every check always, we waste CPU. It's OK to have a test pyramid stopping on failure.
There was a problem hiding this comment.
(OK to externalise this discussion out of PR scope)
There was a problem hiding this comment.
It would have mattered if we paid for CI runners, but we use free ones and cascading it like you described wouldn't make much sense. It would be pretty the same as it was before this PR, with Fixer and tests as the steps in the same job (but in reversed order) - stop on first fail. In my opinion dev should run composer qa before push that would mostly lead to green pipeline, but everyone sometime forget to run it or is super sure everything is alright, just to be surprised in the CI 😉. In this context, I personally would like to get full feedback, not only first failures.
It really would have been different when $$$ should be considered, then requirements and KPIs would be different, leading to different implementation. In this particular case there's no point in queueing jobs that don't technically require it (can work in parallel).
Wirone
force-pushed
the
codito/improve-qa-process
branch
2 times, most recently
from
e31c6bc
to
cef0fb5
Compare
|
@keradus FYI: at this point I just only rebased my branch and resolved conflicts, I'll address comments from the review later. |
It does not make sense to run tests and Fixer on each matrix' combination. We should explicitly run these only when really needed, without repeating same steps on same runtime.
Both tests and Fixer are run twice: - on Symfony ^5 components with `--prefer-lowest` flag for Composer deps - on highest deps available for this PHP version Native type checks are run along with regular Fixer checks because they take only few seconds and there's no need to extract it to separate job - this wouldn't be optimisation.
Wirone
force-pushed
the
codito/improve-qa-process
branch
from
cef0fb5
to
f2c2186
Compare
|
tons of good work, thanks! |
Run tests and Fixer with higher granularity (do not repeat same steps on same runtime). The value of this approach:
Previously PR contained also:
Run dev-tools on PHP 8.2Extracted to chore: Run dev-tools on PHP 8.2 #7389Fix CI warnings caused by installing dev-tools onExtracted to CI: Do not run post-autoload-dump on Composer install #7403post-autoload-dumphook. It did not affect main process, but also it wasn't needed at all, because dev-tools are only used locally and in SCA workflow, where are installed explicitlyBEFORE (~10m):
AFTER (~8m):
Gain: CI time reduced ~20% + more explicit summary.