unit testing improvement #51

Open
wants to merge 8 commits into
from

Projects

None yet

4 participants

@peter279k
peter279k commented Jan 11, 2017 edited

Change log

  • Improve the unit testing and code coverage.
    the travis-ci status is available here.
    the code coverage report is available here.

  • Fix the error in .php_cs.
    In PHP-CS-Fixer version 2.0.0, removing some configurations because they are not available. And fix the class not found error. (please see this issue).

@codecov-io
codecov-io commented Jan 11, 2017 edited

Current coverage is 97.17% (diff: 100%)

No coverage report found for master at 69407bb.

Powered by Codecov. Last update 69407bb...808ec2b

@stof
Contributor
stof commented Jan 11, 2017

if codecov-io adds a comment each time the PR is updated, this is a no-go IMO. It is spammy.

@@ -1,7 +1,5 @@
language: php
-sudo: false
@stof
stof Jan 11, 2017 Contributor

should be reverted

@peter279k
peter279k Jan 11, 2017

Hi @stof, the original configuration is false.
Please see this PR. Thanks.

@stof
stof Jan 11, 2017 Contributor

the default value for sudo is not always false actually. It depends on repositories.

@peter279k
peter279k Jan 11, 2017 edited

Ok, I have been confused this for a long time.
Would you please provide some references about this?
Thanks!

@alcohol
alcohol Jan 11, 2017 Member

It's false for this repo by default though.

@stof
stof Jan 11, 2017 Contributor

@alcohol but it makes it harder to copy-paste the config elsewhere (composer has some repository where it is not the default)

@peter279k
peter279k Jan 12, 2017 edited

Hi all, I think the proper way is adding the sudo: false for each time so that it can always make sure that it's running on the non-root environment.

What do you think?

Thanks.

@stof
stof Jan 12, 2017 Contributor

yeah, I prefer being explicit too, as it does not require knowing at which date the repo started using Travis (this info is not available anywhere except in the Travis database AFAIK, and we cannot access it)

.travis.yml
@@ -20,10 +18,14 @@ php:
matrix:
fast_finish: true
+ allow_failures:
+ - php: hhvm
@stof
stof Jan 11, 2017 Contributor

why ? it is passing in master, so there is no reason to allow it to fail. If your changes are breaking things, you should fix your contribution instead

@peter279k
peter279k Jan 11, 2017

The HHVM is failed because it doesn't support the ReflectionClass.
I use this feature because I want to test some methods directly.

@peter279k
peter279k Jan 12, 2017 edited

Hi @stof, thank you for your suggestions again.
Now the HHVM is fixed and remove the allow_failures.
Thanks.

+ {
+ $expectedString = 'pretty string';
+ $constraint = new \ReflectionClass('\Composer\Semver\Constraint\AbstractConstraint');
+ $setPrettyString = $constraint->getMethod('setPrettyString');
@stof
stof Jan 11, 2017 Contributor

it would be much better to test this through the public API instead

@peter279k
peter279k Jan 11, 2017

As I know, the AbstractConstraintTest is deprecated abstract class so I just use the ReflectionClass to test the trigger_error.

@stof
stof Jan 11, 2017 Contributor

As they are public method, you should just create a child class extending the abstract class and use the public API if you want to cover these methods.

@peter279k
peter279k Jan 12, 2017

Thank you for your tips.
Fix it.

tests/Constraint/EmptyConstraintTest.php
+ protected function tearDown()
+ {
+ unset($this->versionProvide);
+ unset($this->emptyConstraint);
@stof
stof Jan 11, 2017 Contributor

you should not unset the class properties. You should only reset their values (or do nothing as there is no circular dependency with the TestCase, so the refcounting will work fine)

@peter279k
peter279k Jan 11, 2017

You're right. I will fix it later.

@peter279k

Hi @stof. Thank you for your comment, I just want to show the code coverage report so I add the codecov-io service. The owner can remove this and it's up to them. The PR is updated each time that I think it's not a important problem.

@stof
Contributor
stof commented Jan 11, 2017

@peter279k each time there is a comment, maintainers receive an email. Gettings lots of emails just because the service reports the coverage status (which will only change in minor ways) is just spam IMO. github has better ways for automated systems to report an info for the PR (the status API)

@peter279k

Hi @stof, you're right. This should have the spam mail problem.
I think it should show one time and it doesn't show this after the up-coming commit.

- composer install --no-interaction --no-progress --prefer-dist
script:
- - vendor/bin/phpunit
+ - vendor/bin/phpunit -d error_reporting=16384
@stof
stof Jan 12, 2017 Contributor

why ?

@alcohol
alcohol Jan 12, 2017 Member

add -d option for hhvm

Guess it is related to hhvm?

@peter279k
peter279k Jan 12, 2017

Yes. Please refer this issue.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment