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.

Sign up for GitHub

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

Jump to bottom

DX: Improve QA process #7366

Merged
merged 7 commits into from
Nov 2, 2023
Merged

DX: Improve QA process #7366

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e2b1235
CI: Run tests and Fixer with higher granularity
Wirone Oct 10, 2023
76310a0
Explicit requirements for Symfony ^5 / ^6
Wirone Oct 23, 2023
de11bd0
Improve conditions for job matrix
Wirone Oct 26, 2023
8e6af60
CI: run both tests and Fixer after applying migration rules
Wirone Nov 2, 2023
f2c2186
CI: clean up jobs for PHP 7.4
Wirone Nov 2, 2023
8ea7900
Fix tests' execution on PHP 7.4
Wirone Nov 2, 2023
ed126b7
Apply suggestions from code review
keradus Nov 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
115 changes: 91 additions & 24 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,62 +19,128 @@
include:
Wirone marked this conversation as resolved.
Show resolved Hide resolved
Wirone marked this conversation as resolved.
Show resolved Hide resolved
- operating-system: 'ubuntu-20.04'
php-version: '7.4'
job-description: 'with lowest deps'
job-description: 'Fixer with lowest deps'
run-fixer: 'yes'
run-phpdoc-to-native-type: 'yes' # should be checked on the lowest supported PHP version
composer-flags: '--prefer-lowest' # should be checked on the lowest supported PHP version
execute-flex-with-symfony-version: '^5' # explicit check for Symfony 5.x compatibility
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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
?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

there are 20 jobs. we could reduce to ~7job, so 13x15s ~~ 3mins :D

honestly, see no value of keeping "lowest deps" and "sf5" separately

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@Wirone Wirone Nov 2, 2023 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

(OK to externalise this discussion out of PR scope)

Copy link
Member Author

Choose a reason for hiding this comment

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

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).


- operating-system: 'ubuntu-20.04'
php-version: '7.4'
job-description: 'with PHPDoc to type rules'
phpdoc-to-type-rules: 'yes' # should be checked on the lowest supported PHP version
job-description: 'tests with lowest deps'
run-tests: 'yes'
composer-flags: '--prefer-lowest' # should be checked on the lowest supported PHP version
execute-flex-with-symfony-version: '^5' # explicit check for Symfony 5.x compatibility

- operating-system: 'ubuntu-20.04'
php-version: '7.4'
job-description: 'with Symfony ^5'
execute-flex-with-symfony-version: '^5' # explicit check for Symfony 5.x compatibility
job-description: 'Fixer'
run-fixer: 'yes'
run-phpdoc-to-native-type: 'yes' # should be checked on the lowest supported PHP version

- operating-system: 'ubuntu-20.04'
php-version: '7.4'
job-description: 'tests'
run-tests: 'yes'
Wirone marked this conversation as resolved.
Show resolved Hide resolved

- operating-system: 'ubuntu-20.04'
php-version: '8.0'
job-description: 'Fixer'
run-fixer: 'yes'

- operating-system: 'ubuntu-20.04'
php-version: '8.0'
job-description: 'tests'
run-tests: 'yes'

- operating-system: 'ubuntu-20.04'
php-version: '8.1'
job-description: 'with Symfony ^6'
job-description: 'Fixer with Symfony ^6'
run-fixer: 'yes'
execute-flex-with-symfony-version: '^6' # explicit check for Symfony 6.x compatibility

- operating-system: 'ubuntu-20.04'
php-version: '8.1'
job-description: 'tests with Symfony ^6'
run-tests: 'yes'
execute-flex-with-symfony-version: '^6' # explicit check for Symfony 6.x compatibility

- operating-system: 'ubuntu-20.04'
php-version: '8.2'
job-description: 'Fixer'
run-fixer: 'yes'

- operating-system: 'ubuntu-20.04'
php-version: '8.2'
job-description: 'Fixer with migration rules'
run-fixer: 'yes'
run-migration-rules: 'yes' # should be checked on the highest supported PHP version
Wirone marked this conversation as resolved.
Show resolved Hide resolved

- operating-system: 'ubuntu-20.04'
php-version: '8.2'
job-description: 'with migration rules'
migration-rules: 'yes' # should be checked on the highest supported PHP version
job-description: 'tests with migration rules'
run-tests: 'yes'
run-migration-rules: 'yes' # should be checked on the highest supported PHP version

- operating-system: 'ubuntu-20.04'
php-version: '8.2'
job-description: 'auto-review'
run-tests: 'yes'
phpunit-flags: '--group auto-review'

- operating-system: 'ubuntu-20.04'
php-version: '8.2'
job-description: 'with calculating code coverage'
calculate-code-coverage: 'yes'
job-description: 'tests'
run-tests: 'yes'

- operating-system: 'ubuntu-20.04'
php-version: '8.2'
job-description: 'with deployment'
execute-deployment: 'yes'
job-description: 'code coverage'
code-coverage: 'yes'
Wirone marked this conversation as resolved.
Show resolved Hide resolved

- operating-system: 'ubuntu-20.04'
php-version: '8.3'
PHP_CS_FIXER_IGNORE_ENV: 1
composer-flags: '--ignore-platform-req=PHP'
php-version: '8.2'
job-description: 'deployment check'
execute-deployment: 'yes'

- operating-system: 'windows-latest'
php-version: '8.2'
job-description: 'on Windows'
job-description: 'Fixer on Windows'
run-fixer: 'yes'
FAST_LINT_TEST_CASES: 1 # we need full syntax check on one job at least, no need to do it on additional

- operating-system: 'windows-latest'
php-version: '8.2'
job-description: 'tests on Windows'
run-tests: 'yes'
FAST_LINT_TEST_CASES: 1 # we need full syntax check on one job at least, no need to do it on additional systems

- operating-system: 'macos-latest'
php-version: '8.2'
job-description: 'on macOS'
job-description: 'Fixer on macOS'
run-fixer: 'yes'
FAST_LINT_TEST_CASES: 1 # we need full syntax check on one job at least, no need to do it on additional systems

- operating-system: 'macos-latest'
php-version: '8.2'
job-description: 'tests on macOS'
run-tests: 'yes'
FAST_LINT_TEST_CASES: 1 # we need full syntax check on one job at least, no need to do it on additional

- operating-system: 'ubuntu-22.04'
php-version: '8.3'
job-description: 'Fixer'
run-fixer: 'yes'
composer-flags: '--ignore-platform-req=PHP'
PHP_CS_FIXER_IGNORE_ENV: 1

- operating-system: 'ubuntu-22.04'
php-version: '8.3'
job-description: 'tests'
run-tests: 'yes'
composer-flags: '--ignore-platform-req=PHP'
PHP_CS_FIXER_IGNORE_ENV: 1

name: PHP ${{ matrix.php-version }} ${{ matrix.job-description }}

runs-on: ${{ matrix.operating-system }}
Expand All @@ -87,7 +153,7 @@
uses: actions/github-script@v6
id: code-coverage-driver
with:
script: 'return "${{ matrix.calculate-code-coverage }}" == "yes" ? "pcov" : "none"'
script: 'return "${{ matrix.code-coverage }}" == "yes" ? "pcov" : "none"'
Wirone marked this conversation as resolved.
Show resolved Hide resolved
result-encoding: string

- name: Setup PHP
Expand Down Expand Up @@ -125,43 +191,44 @@
max_attempts: 5
retry_wait_seconds: 30
command: |
composer update --optimize-autoloader --no-interaction --no-progress --no-scripts ${{ matrix.composer-flags }} # --no-scripts to avoid installing dev-tools for all jobs on CI level

Check warning on line 194 in .github/workflows/ci.yml

View workflow job for this annotation

GitHub Actions / Validate YAML 

194:181 [line-length] line too long (192 > 180 characters)
composer info -D

# execute migration rules before running tests and self-fixing,
# so that we know that our codebase is future-ready
- name: Run PHP CS Fixer with migration rules
if: matrix.migration-rules == 'yes'
if: matrix.run-migration-rules == 'yes'
run: php php-cs-fixer fix --config .php-cs-fixer.php-highest.php -q

- name: Disable time limit for tests when collecting coverage
if: matrix.calculate-code-coverage == 'yes'
if: matrix.code-coverage == 'yes'
run: sed 's/enforceTimeLimit="true"/enforceTimeLimit="false"/g' phpunit.xml.dist > phpunit.xml

- name: Run tests
if: matrix.calculate-code-coverage != 'yes'
if: matrix.run-tests == 'yes'
Wirone marked this conversation as resolved.
Show resolved Hide resolved
env:
PHP_CS_FIXER_IGNORE_ENV: ${{ matrix.PHP_CS_FIXER_IGNORE_ENV }}
FAST_LINT_TEST_CASES: ${{ matrix.FAST_LINT_TEST_CASES }}
run: vendor/bin/paraunit run ${{ matrix.phpunit-flags || '--exclude-group auto-review' }}

- name: Collect code coverage
if: matrix.calculate-code-coverage == 'yes'
if: matrix.code-coverage == 'yes'
env:
FAST_LINT_TEST_CASES: 1
Wirone marked this conversation as resolved.
Show resolved Hide resolved
run: vendor/bin/paraunit coverage --testsuite coverage --exclude-group covers-nothing --clover build/logs/clover.xml

- name: Upload coverage results to Coveralls
if: matrix.calculate-code-coverage == 'yes'
if: matrix.code-coverage == 'yes'
env:
COVERALLS_REPO_TOKEN: ${{ secrets.GITHUB_TOKEN }}
run: php vendor/bin/php-coveralls --verbose

- name: Run PHP CS Fixer with PHPDoc to type rules
if: matrix.phpdoc-to-type-rules == 'yes'
if: matrix.run-phpdoc-to-native-type == 'yes'
run: php php-cs-fixer check --diff -v --config .php-cs-fixer.php-lowest.php

- name: Run PHP CS Fixer
if: matrix.run-fixer == 'yes'
env:
PHP_CS_FIXER_IGNORE_ENV: ${{ matrix.PHP_CS_FIXER_IGNORE_ENV }}
PHP_CS_FIXER_FUTURE_MODE: 1
Expand Down