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

Move phpunit.xml to root folder #166

Merged
merged 3 commits into from
Nov 26, 2018
Merged

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Nov 22, 2018

  • Put phpunit.xml into the root folder of the app.
  • Put the coverage report into tests/output/clover.xml
  • Use the phpunit from core
  • Remove excess items from require-dev of composer.json

@phil-davis
Copy link
Contributor Author

@PVince81 @DeepDiver1975 this seems to be what has been done in other apps.

Is this the current "standard"?

If so, then we can easily adjust other apps the same way.

@codecov
Copy link

codecov bot commented Nov 22, 2018

Codecov Report

Merging #166 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #166   +/-   ##
=========================================
  Coverage     72.83%   72.83%           
  Complexity      225      225           
=========================================
  Files            25       25           
  Lines           968      968           
=========================================
  Hits            705      705           
  Misses          263      263

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1098a21...2439a7c. Read the comment docs.

@phil-davis
Copy link
Contributor Author

phil-davis commented Nov 22, 2018

@DeepDiver1975 please review and approve, assuming your thumbs-up above means you are happy.

@phil-davis phil-davis force-pushed the move-phpunit-xml-to-root branch 2 times, most recently from 8755275 to a736598 Compare November 23, 2018 08:18
@phil-davis
Copy link
Contributor Author

@DeepDiver1975 @patrickjahns @PVince81 this is ready for review again.
I got rid of the local phpunit and it uses phpunit from core, in the same way that data_exporter is doing.

@phil-davis
Copy link
Contributor Author

This makes the CI infrastructure in this app more like "the imagined standard".
It implements stuff like what is already in data_exporter

@patrickjahns this one will be easier to review (smaller) than the files_antivirus one. So it might be good to get this agreed and merged first. Then we just check that owncloud/files_antivirus#273 implements things the same way.

@@ -5,14 +5,15 @@
# Composer
composer.phar
vendor/
vendor-bin/**/vendor
Copy link
Member

Choose a reason for hiding this comment

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

when we have vendor/ in .gitignore file, do we really need vendor-bin/**/vendor

Copy link
Contributor Author

@phil-davis phil-davis Nov 23, 2018

Choose a reason for hiding this comment

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

vendor/ will just ignore the directory vendor at the top level of the repo.
The other one is inside vendor-bin/ which is a different top-level directory.

Edit: see comments below

Copy link
Member

@paurakhsharma paurakhsharma Nov 23, 2018

Choose a reason for hiding this comment

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

@phil-davis, IMO if we add /vendor it will only ignore the top-level directory. But in this case (i.e when using vendor/) every vendor folder inside the repo is ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested this just now. What @paurakhsharma says is true. Every folder called vendor anywhere in the tree is ignored just by writing vendor/ in .gitignore

@patrickjahns please advise what you want to do. Because we need to have a consistent format for this stuff in every repo. Otherwise it is confusing for people who wonder about small differences.

@DeepDiver1975 DeepDiver1975 merged commit 61c62b2 into master Nov 26, 2018
@DeepDiver1975 DeepDiver1975 deleted the move-phpunit-xml-to-root branch November 26, 2018 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants