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

Phpunit integration && PHP 7 update #4

Open
wants to merge 10 commits into
base: trunk
Choose a base branch
from

Conversation

jclaveau
Copy link

@jclaveau jclaveau commented Jan 26, 2020

Hello, your work seems to still be the best solution for Apache configuration parsing in PHP. As I try to keep safe libs in my projects, I integrated PHPUnit and updated your lib for PHP 7.

I still have one failing test: bug11435-inifile.phpt
But I'm not sure of what I have to do. I need a little help for this case.

This PR could seem big in appearance but is quit simple in fact. I just moved all the old test files to a dedicated directory. All the real modifications to the "business" code are gathered in the first commit: 1a2c2fc

Thanks in advance for your time

@jclaveau
Copy link
Author

Also, if this work fits your expectations, would you please enable this repo in Travis for continuous integration?

@@ -2,3 +2,6 @@
composer.lock
composer.phar
vendor
tests/coverage
.phpunit.result.cache
clover.xml
Copy link
Member

Choose a reason for hiding this comment

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

These should have went into your global .gitignore.

Copy link
Author

Choose a reason for hiding this comment

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

tests/coverage and clover.xml are specified in the phpunit.xml. Are you sure they should be global?

Copy link
Member

@sanmai sanmai Feb 7, 2020

Choose a reason for hiding this comment

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

I have .phpunit.result.cache in pretty much in every other project. Two others are not belong here in the first place. E.g. you can put them into a build folder, then ignore it globally. Or add a local .gitignore with a * in that folder. But I'm just nitpicking here. Great job!

Copy link
Author

Choose a reason for hiding this comment

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

I agree with you concerning .phpunit.result.cache but for the 2 others adding a dedicated folder which itself is ignored looks just as adding out of scope complexity. It could be necessary, if ever a full build process had to be configured, but later, when the build folder would gather more stuff. And taking this in consideration here seems much less important than considering coverage for example.

If you have an example of a build process applied to other Pear packages, I would follow it gladly. Is it the case?

.travis.yml Outdated Show resolved Hide resolved
Config/Container.php Outdated Show resolved Hide resolved
tests/AbstractTest.php Outdated Show resolved Hide resolved
@jclaveau
Copy link
Author

jclaveau commented Feb 6, 2020

Moreover, would you also enable this repo on https://codecov.io (or any coverage saas) so I could enable it in Travis (passing tests means nothing without coverage)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants