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

Test enhancement about some PHPUnit stuffs #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

peter279k
Copy link

@peter279k peter279k commented Jul 28, 2020

Changed log

  • Add php-7.4 version test during Travis CI build.
  • The carbondate/carbon package is abandoned/deprecated. Using the nesbot/carbon package instead.
  • To be consistency, using the self approach to make PHPUnit assertion call.
  • Using the $this approach to make expectException method call.
  • To avoid deprecated class loading issue, it should remove classmap-authoritative config inside composer.json file.
  • According to the PHPUnit fixtures reference, it should be nice to use protected function setUp(): void method.

This change is Reviewable

@@ -144,7 +144,7 @@ public function testDataCannotBeNullByDefault()
'name' => 'string',
];

self::expectException(InvalidDataTypeException::class);
$this->expectException(InvalidDataTypeException::class);
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be reverted back to self:: for self-sufficiency.

Copy link
Author

Choose a reason for hiding this comment

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

Actually, it should be $this to call expectException method.

And it only has the two method call about assertion methods.

Some references are available here:

But I think it's fine to use self to call expectException method, too.

I will revert this :).

Copy link
Author

Choose a reason for hiding this comment

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

Hi @hopeseekr, thanks for your comment.

And I've reverted this about using the self to do expectException method call :).

@peter279k peter279k requested a review from hopeseekr August 5, 2020 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants