-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Cleanup difficult to maintain Travis setup and addition of enhancements #3567
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
Cleanup difficult to maintain Travis setup and addition of enhancements #3567
Conversation
…(done in vm setup already)
- moves everything into one place, the before_script.sh - now removes `update/dev/tests/integration/testsuite` set from all but test set 1 - integration set handling should be flexible by configuration env vars in single location (.travis.xml) - fixed an oops in d2884f0 (accidental change to matrix exclusions)
- this is preferable given the test suite preparation being done on the fly for integration testing
…won't run outside of a directory during split testing on travis)
cant say much to the actual changes, as mixing them with the refactoring makes it to much work for me. But a comment to the Github Token. |
@Flyingmana Good point. Unfortunately if the env vars are set securely (i.e. hidden from logs, etc) they are not available to builds running for PRs though. Does it make a difference in your mind knowing that there is still a rate limit in place when using an API access token? The authenticated rate-limit is set at 5000 requests per hour vs the unauthenticated limit of 60 per hr per IP address. |
There are some open issues about using direct links from Github, so hopefully it soon isn't needed anymore to use the Github tokens: composer/composer#4944 and composer/composer#4884 |
Also, just curious, why do you need to install a custom composer version? It that still needed? According to docs, they're updated every 30-60 days: https://docs.travis-ci.com/user/languages/php#Installing-Composer-packages |
dev/travis/script.sh
Outdated
;; | ||
static) | ||
cd dev/tests/static | ||
phpunit --filter 'Magento\\Test\\Php\\LiveCodeTest::(testCodeStyle|testAnnotationStandard)' |
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.
These tests were intentionally broken down into two parts because they took much time. Right now they are fast because, looks like, they just don't test anything. So I'd leave these tests separate, as it may happen that they will become slow again when fixed.
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.
That would make sense. Would it be acceptable to keep as it is here and expect that when the tests are updated to actually run against the changed files, this can be split again if that performance issue is run into (it would be a very minor change to split them at that point)?
Based on my review, these will only run the static tests on files changed in the current PR, but they are not actually doing anything because the changed file lists aren't being populated, and so I suspect that they may not really take longer than the max job run times when setup correctly.
I'm willing to tackle getting those change lists populating (separate PR, after this is merged in) so we can have them running on travis builds, and if any speed issues are encountered can easily split them.
Let me know your thought, and if you'd rather I just leave them split for now, I can do that too.
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.
Actually @mazhalai is already working on fixing static tests. Example can be found here - https://travis-ci.org/mazhalai/magento2/builds/115163946#L746 and https://travis-ci.org/mazhalai/magento2/jobs/115362517#L594 . But thanks for the help :)
It looks pretty fast now, but it may take a lot of time for phpmd in case of changes across whole code base (rare case, but still may happen).
I don't insist, though I don't see a big benefit from merging these tests together.
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.
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.
Interesting, that @mazhalai has also splitted them... :) while we were discussing it here.
Let's leave it as is then and we'll merge all together, when will be merging the PR and internal changed together.
Sorry for this long discussion.
8f9903c
to
3e8865f
Compare
Internal ticket to process this: MAGETWO-50237 |
@davidalger it seems like the tests are timing out. Could you please take a look |
734db34
to
f36c3c7
Compare
@davidalger, please update with the latest code base and resolve conflicts |
This reverts commit f36c3c7.
…ravis-mods Conflicts: - .travis.yml (changes made to this file moved into dev/travis/script.sh) - dev/tests/integration/IntegationTestsForTravis.sh (only incoming change was the copyright date change) - dev/tests/integration/phpunit.xml.dist
4d927db
to
41c7da3
Compare
3d5db11
to
1d149b5
Compare
eed4b38
to
3bba105
Compare
…ce reason for introduction is now mute
3bba105
to
8b18349
Compare
@arkadiych Done. The updates to the static tests made by @mazhalai have been incorporated, and everything built and passed as green. Let me know if you need anything else on this to get it merged in. I'd love to see it make it into the mainline soon! :) |
@davidalger Thank you for contribution. Your Pull Request merged to develop branch. |
[TSG-CSL3] For 2.2 (happy New Year ;)
Cleans up
.travis.yml
by movingbefore_install
,before_script
andscript
portions to .sh filesAll script components are now in a single location (specifically
dev/travis
)Static tests and annotation tests are no longer performed twice (excluded from PHP 7.0 as they were for PHP 5.6)
Using builtin travis config to install apt packages such as mysql 5.6 and postfix
Integration test splitting is now 100% maintained by the env vars configured in .travis.yml (
INTEGRATION_INDEX
andINTEGRATION_SETS
)Added support for using a
GITHUB_TOKEN
environment variable to speed up buildsThis in combination with setting a
public_access
API token on the repository will speed up the test suite by about 20 minutes by cutting ~85 seconds off each call tocomposer install
since with this token the deps may be installed from the distribution vs using a cloneI've set a
public_access
GitHub API O-Auth token on themagento/magento2
repository. It will show up in builds, but per the documentation is read-only and limited to public information:UPDATE: Now includes the following as well:
testsuite/Magento/Test/Integrity
tests into main integration buildstatic_phpcs
andstatic_annotation
into a single build in the matrixphpunit
configuration file load behavior vs explicit --configuration input