[TASK] Move the unit tests from Travis CI to GitHub actions #846
Conversation
Removing the Travis CI integration will follow in a separate commit. Part of #818.
oliverklee
force-pushed
the
task/move-unit-tests
branch
from
dcb86ae to
5cd3470
Compare
.github/workflows/ci.yml
Outdated
| if [[ "${{ matrix.dependencies }}" == 'lowest' ]]; then | ||
| DEPENDENCIES='--prefer-lowest'; | ||
| fi; |
There was a problem hiding this comment.
Should there be an else case to set DEPENDENCIES to an empty string otherwise, just in case?
There was a problem hiding this comment.
Yes, that would be cleaner. I'll update the PR accordingly.
| if [[ "${{ matrix.dependencies }}" == 'lowest' ]]; then | ||
| DEPENDENCIES='--prefer-lowest'; | ||
| fi; | ||
| composer update --no-progress "${DEPENDENCIES}"; |
There was a problem hiding this comment.
In #638 the --ignore-platform-reqs argument was added for PHP 7.3 (and later 7.4) because friendsofphp/php-cs-fixer did not explicitly support PHP 7.3. Although not needed by the unit tests, it's nonetheless a 'dev' dependency in composer.json. I'm guessing this is not required any more because PHP CS Fixer now supports the latest PHP versions.
There was a problem hiding this comment.
Exactly. (This kind of problem will go away once we've moved the tools from Composer dependencies to Phive.)
| php${{ matrix.php-version }}-${{ matrix.dependencies }}-composer- | ||
|
|
||
| - name: Install Composer dependencies | ||
| run: | |
There was a problem hiding this comment.
Might a composer install step be needed first, before composer update?
There was a problem hiding this comment.
No, this is not required (and would slow down the process as we'd then potentially download two versions of some packages). update always includes the install (and for calculating the updates, Composer does not need any packages installed.)
| uses: shivammathur/setup-php@v1 | ||
| with: | ||
| php-version: ${{ matrix.php-version }} | ||
| ini-values: xdebug.max_nesting_level=500 |
There was a problem hiding this comment.
Do we need this (we did not previously change the setting)?
There was a problem hiding this comment.
Yes. Without this setting, we'd get a "maximum nesting level reached" error in CssInlinerTest::inlineCssAppliesMoreSpecificCssSelectorToMatchingElements:
https://github.com/MyIntervals/emogrifier/runs/539483776?check_suite_focus=true
Removing the Travis CI integration will follow in a separate commit.
Part of #818