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

[8.x] Add tests for getting the access token with the password grant #1181

Merged

Conversation

X-Coder264
Copy link
Contributor

@X-Coder264 X-Coder264 commented Feb 8, 2020

After adding the support for feature tests in #1155 I'm sending this PR which contains feature tests for getting the access token with the password grant.

If this gets merged I'll probably also add the tests for the other grants when I'll have the time.

@X-Coder264 X-Coder264 force-pushed the add-feature-tests-for-password-grant branch 2 times, most recently from 519f087 to fe0004b Compare February 8, 2020 21:27
@X-Coder264 X-Coder264 force-pushed the add-feature-tests-for-password-grant branch 2 times, most recently from f5c4768 to d5f7cdc Compare February 9, 2020 00:48
@@ -20,4 +20,8 @@
<directory suffix=".php">./src/</directory>
</whitelist>
</filter>

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

@driesvints
Copy link
Member

This all looks pretty promising. Thanks for your work on this 👍

@taylorotwell taylorotwell merged commit 0920021 into laravel:8.x Feb 10, 2020
@X-Coder264 X-Coder264 deleted the add-feature-tests-for-password-grant branch February 10, 2020 14:30
@driesvints
Copy link
Member

@X-Coder264 any reason in particular why you used mysql instead of sqlite?

@X-Coder264
Copy link
Contributor Author

@driesvints There's no particular reason, it was just so easy to setup mysql that I saw no reason not to (especially since sqlite is not a database that is used in production).

@driesvints
Copy link
Member

@X-Coder264 we use sqlite for all our testing in all libraries so it might be best that we switch that. I can't for example run the tests suite locally now out of the box without having to setup a mysql database. Can you send in a new PR to change that maybe?

@X-Coder264
Copy link
Contributor Author

I actually did this to be just like how laravel/framework is tested, but yeah, I see your point. I can send a PR for this sometime tomorrow probably as I'm swamped today.

@driesvints
Copy link
Member

I've updated the test suite to use sqlite instead.

@X-Coder264
Copy link
Contributor Author

@driesvints Sorry for not doing it myself, I had a ton of work, but I hadn't forgotten it, it was on my TODO list for the weekend. Thanks for taking care of it.

@driesvints
Copy link
Member

@X-Coder264 no problem at all :)

$this->assertArrayHasKey('refresh_token', $decodedResponse);
$this->assertSame('Bearer', $decodedResponse['token_type']);
$expiresInSeconds = 31622400;
$this->assertEqualsWithDelta($expiresInSeconds, $decodedResponse['expires_in'], 5);
Copy link
Member

Choose a reason for hiding this comment

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

@X-Coder264 this line was failing for us randomly. I had to update the value of the $expiresInSeconds variable to 31536000 instead. Any idea why? I also don't really know what we're testing here exactly? Why should the expires_in be the value of above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@driesvints This tests the expiration time of the issued access token. The expiration time for the password grant is being set here:

https://github.com/laravel/passport/blob/v8.4.0/src/PassportServiceProvider.php#L116

This calls Passport::tokensExpireIn() which then returns new DateInterval('P1Y') (because there is no $date provided and its default value is null and static::$tokensExpireAt is null too).

The difference between our numbers is 86400 seconds which is exactly one day. I guess there is some weird time related stuff going on with that DateInterval which causes a random fail of that specific assertion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did the test maybe start to fail after 29.02.? If so then it is related to the leap year which is exactly why there was a one day difference between our expiration numbers 😄

Copy link
Member

Choose a reason for hiding this comment

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

hah, that could be yeah

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

Successfully merging this pull request may close these issues.

3 participants