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

Merged all travis configuration into one file to improve maintainability #678

Merged
merged 1 commit into from
Jan 24, 2017

Conversation

Bilge
Copy link
Contributor

@Bilge Bilge commented Jan 22, 2017

Merging all the Travis configuration into one file gives us a better overview of what is actually happening and thus improves maintainability of the Travis build. This configuration is mostly unchanged from the original, but as an added bonus, installation of satooshi/php-coveralls was moved to the after_success step because it is not needed of the build fails and coverage is never generated, thus speeding up the build.

There are still quirks, errors and optimizations to be done but I will submit them after this has been merged. For example:

  • Question: Why is only PHP 5.6 allowed to upload code coverage? Surely every build should be recorded.
  • Question: Why is MockeryPHPUnitIntegration stripped from the PHP 5.6 build?
  • Bug: PHP 7.1 still generates code coverage even though it's not uploaded.
  • Optimization: The same tests against PHP version are duplicated in several different steps.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 76.273% when pulling 0287817 on Bilge:travis-onefile into 897c2e7 on padraic:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 76.273% when pulling a91fb09 on Bilge:travis-onefile into 897c2e7 on padraic:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 76.273% when pulling 54265e7 on Bilge:travis-onefile into 897c2e7 on padraic:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 76.273% when pulling 328cd48 on Bilge:travis-onefile into 897c2e7 on padraic:master.

@davedevelopment
Copy link
Collaborator

Question: Why is only PHP 5.6 allowed to upload code coverage? Surely every build should be recorded.

I assume that's because the services we upload the coverage to have 1 build per commit, rather than travis which gives you multiple "sub" builds. I don't know much about coveralls and I've not used the coverage uploader for scrutinizer before either.

Question: Why is MockeryPHPUnitIntegration stripped from the PHP 5.6 build?

7073e80 suggests it was originally excluded from other builds, as they didn't want the trait looked at for coverage or something, that part of the travis script is putting it back in the build I guess. We can strip it out of master as we've move to >5.6.

I take it this supersedes #677?

👍 from me, I don't care for the separate files.

@Bilge
Copy link
Contributor Author

Bilge commented Jan 23, 2017

@davedevelopment Well spotted, this does indeed incorporate the changes from #677. Therefore, please merge this instead to avoid conflicts.

I assume that's because the services we upload the coverage to have 1 build per commit

That is definitely not the case for Coveralls. Not sure about Scrutinizer, but from what I recall, it is not true for that service either. Both Coveralls and Scrutinizer are built with Travis integration in mind and Travis has supported multiple builds per commit since the beginning. I may address this in a future PR.

that part of the travis script is putting it back in the build I guess

The opposite. The final d of sed '/MockeryPHPUnitIntegration/d' means delete and therefore removes any matching lines. Thus this test is run for all versions except 5.6. Ergo this strategy still does not make any sense to me.

I don't care for the separate files.

It occurred to me whilst merging there exists the possibility that the original author did not, either. Perhaps they just did not know how to express their script in YAML format.

@davedevelopment davedevelopment merged commit 9c8e39d into mockery:master Jan 24, 2017
@davedevelopment
Copy link
Collaborator

Merged, thanks!

The opposite. The final d of sed '/MockeryPHPUnitIntegration/d' means delete and therefore removes any matching lines. Thus this test is run for all versions except 5.6. Ergo this strategy still does not make any sense to me.

Are you sure? I know the d means delete, but it's deleting it from an exclude filter in the PHPUnit config.

@Bilge Bilge deleted the travis-onefile branch January 24, 2017 09:51
@Bilge Bilge restored the travis-onefile branch January 24, 2017 09:51
@Bilge
Copy link
Contributor Author

Bilge commented Jan 24, 2017

it's deleting it from an exclude filter in the PHPUnit config

Ah. I didn't actually look at the config! I still don't understand it at all. I see no reason why it should be excluded, particularly on a per-version basis, so I think it can just be removed right now on master.

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.

None yet

3 participants