-
Notifications
You must be signed in to change notification settings - Fork 9
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
Configure strict testing via phpunit.xml. #33
Conversation
See dominionenterprises/util-php#32 and dominionenterprises/util-php#33.
See dominionenterprises/util-php#32 and dominionenterprises/util-php#33.
See dominionenterprises/util-php#32 and dominionenterprises/util-php#33.
'coverageHtml' => 'coverage', | ||
'configuration' => $phpunitConfiguration, | ||
); | ||
$phpunitArguments = array('coverageHtml' => 'coverage', 'configuration' => $phpunitConfiguration); |
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.
coverage-html can also be specified in the xml file.
<logging>
<log type="coverage-html" target="coverage"/>
</logging>
That being said, I know in the past I've been a huge proponent of the coverage-html
, however its not needed for the automated builds and can easily be specified at command line when needed. My vote is to remove it.
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.
If scrutinizer
needs the coverage html files, then obviously leave it in.
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.
Problem with removing it is that we would also likely want to remove it from .gitignore
which makes generating the report less fun because it leaves an untracked directory around. I like it more where it is now as we can just run phpunit
directly (./vendor/bin/phpunit
) with no args if we don't want to do coverage.
Also, build.php
needs a coverage report in order to run (the code coverage report is only generated if some code coverage report is specified). So removing coverage-html could only be done by instead adding something else.
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.
If scrutinizer needs the coverage html files, then obviously leave it in.
scrutinizer runs phpunit itself (it does not run build.php) with its own arguments for coverage reports.
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.
gotcha.
@Gaillard, what are your thoughts? |
'enforceTimeLimit' => true, | ||
'disallowTodoAnnotatedTests' => true, | ||
'coverageHtml' => 'coverage', | ||
'configuration' => $phpunitConfiguration, |
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.
you only added strict to the phpunit.xml, what about the other options here like disallowTestOutput or the todo one?
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.
I think its fine leaving coverage only in the build and not in the phpunit.xml so that one can use and the other not, but the other options were changed 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.
--strict
does all of those things like Chad linked to in the filter-php PR. strict=true
is a wider option that when run through the configuration parser and CLI argument parser gets expanded to this full set of options (lines 20-24 in the old file here).
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.
ok, stick that in the commit message then
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.
done.
This `strict="true"` option sets the `reportUselessTests`, `strictCoverage`, `disallowTestOutput`, `enforceTimeLimit`, and `disallowTodoAnnotatedTests` options when run through the configuration parser. It will also add other new strict options as phpunit upstream adds them.
Configure strict testing via phpunit.xml.
See dominionenterprises/util-php#32 and dominionenterprises/util-php#33.
No description provided.