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

Make Travis fast again #709

Merged
merged 3 commits into from
Aug 29, 2016
Merged

Make Travis fast again #709

merged 3 commits into from
Aug 29, 2016

Conversation

teohhanhui
Copy link
Contributor

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets N/A
License MIT
Doc PR N/A
  • Use phpdbg instead of xdebug for code coverage
  • Speed up npm install

But we need to verify that code coverage is working properly.

@teohhanhui teohhanhui force-pushed the travis-perf branch 2 times, most recently from b25d6d6 to a2c5501 Compare August 29, 2016 06:08
@teohhanhui
Copy link
Contributor Author

Ok the coverage stats seems to be different between xdebug and phpdbg, but I think we're good?

ping @api-platform/core-team


env:
global:
- NPM_CONFIG_PROGRESS='false'
Copy link
Member

Choose a reason for hiding this comment

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

Is this still true, thought they fixed it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't seem to have an effect in Travis CI environment anyway, since there's no TTY 😛

But there's no harm...

Copy link
Member

Choose a reason for hiding this comment

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

Sure! Shrinkwrap does speed up things but I'm not sure we've a use case here.

Copy link
Member

@dunglas dunglas Aug 29, 2016

Choose a reason for hiding this comment

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

If it is useless, can you remove it? (simpler is better)

@dunglas
Copy link
Member

dunglas commented Aug 29, 2016

Awesome

- phpunit --self-update
- npm install -g swagger-cli
- if [[ $coverage = 1 ]]; then mkdir -p build/logs build/cov; fi
- if [[ $coverage = 1 ]]; then wget https://phar.phpunit.de/phpcov.phar; fi
- if [[ $coverage = 1 ]]; then wget https://github.com/satooshi/php-coveralls/releases/download/v1.0.1/coveralls.phar; fi

install:
- if [[ ! $deps ]]; then composer update --no-progress --ansi; fi
- if [[ $deps = low ]]; then composer update --no-progress --ansi --prefer-lowest --prefer-stable; fi
- if [[ $coverage = 1 ]]; then composer require --dev --no-update 'phpunit/php-code-coverage:^4.0.1'; fi
Copy link
Member

Choose a reason for hiding this comment

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

We should either download the phar or install it using composer, not both :)

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe I missed something. Why is this lib necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not a new dependency. We're using it in CoverageContext. It was used through a transitive dependency, which is of course something we shouldn't do. (I really hope we have a dependency analysis tool to help us catch such problems!)

Shall we add it to require-dev in composer.json proper?

Copy link
Member

Choose a reason for hiding this comment

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

It looks good to me to have it in require-dev.

@teohhanhui
Copy link
Contributor Author

teohhanhui commented Aug 29, 2016

@dunglas After adding "phpunit/php-code-coverage": "^4.0.1" to require-dev the deps='low' build is failing. I'm not quite sure how we should proceed.

@dunglas
Copy link
Member

dunglas commented Aug 29, 2016

It's probably related to minimum versions of PHPUnit's dependencies. Their minimum version should be explicitly added to composer.json, but it's probably enough for now to add this dep in Travis like you've done previously.

@teohhanhui
Copy link
Contributor Author

Not a satisfactory solution but that is something to be solved in another PR. 😄

@dunglas dunglas merged commit c7c64b7 into api-platform:master Aug 29, 2016
@dunglas
Copy link
Member

dunglas commented Aug 29, 2016

Thanks @teohhanhui

@teohhanhui teohhanhui deleted the travis-perf branch August 30, 2016 03:35
magarzon pushed a commit to magarzon/core that referenced this pull request Feb 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants