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

Allow time to be set for test purposes #852

Merged
merged 7 commits into from
Oct 28, 2020

Conversation

kevinquinnyo
Copy link
Contributor

This just adds a static method for test purposes so that downstream code can more easily assert an exact AccessToken object in their tests.

Since the calls to time() are non-deterministic, downstream tests are forced to mock, or do looser assertions on the expires property.

Usage might be something like this:

// in a test in some codebase that uses this library
public function tearDown()
{
    parent::tearDown();
    AccessToken::resetTimeNow();
}

public function testSomething()
{
    AccessToken::setTimeNow(1000);

    // $something = ... build object under test that uses this library to generate AccessToken
    // $expectedToken = new AccessToken(...)

    $token = $something->getToken();
    $this->assertEquals($token, $expectedToken);
}

I did this because I was writing some tests around code that depends on this library, and it seemed useful to me, however I understand if using static methods for test purposes is something you prefer not to do, or it otherwise doesn't fit with the coding preferences here.

kevinquinnyo and others added 5 commits August 6, 2020 14:46
This just adds a static method for test purposes so that downstream code
can more easily assert an exact `AccessToken` object in their tests.

Since the calls to `time()` are non-deterministic, downstream tests are
forced to mock, or do looser assertions on the `expires` property.
And write a few more tests around new method
@kevinquinnyo
Copy link
Contributor Author

@ramsey I had forgotten about this PR, but I got the notification master had been merged in, so I checked it and noticed that there was a CI failure due to TestCase::tearDown() not matching the parent which has a return typehint of void. So I sent up a fix for that real quick.

But fixing this makes the php 5.6 run fail. Seems like whack-a-mole. What's the solution here?

@ramsey
Copy link
Contributor

ramsey commented Oct 27, 2020

I dealt with the same thing getting tests to run on 5.6 through 8.0. The solution is to code around setUp() and tearDown(). Figure out an alternative to using it. Maybe change the name of the function and then call it at the end of every test method? It sounds silly, but that's probably the best solution for now.

@kevinquinnyo
Copy link
Contributor Author

wack-a-mole complete lol 🔨

@ramsey
Copy link
Contributor

ramsey commented Oct 28, 2020

Thanks!

@ramsey ramsey merged commit 2392bf7 into thephpleague:master Oct 28, 2020
@kevinquinnyo kevinquinnyo deleted the set-time-now branch October 29, 2020 02:03
@dpi
Copy link

dpi commented Nov 8, 2020

@ramsey FWIW there is a strategy that Symfony and Drupal have been using to overcome PHP compatibility issues when trying to support PHP5 + newer versions of PHPunit:

On test bootstrap, actually modify the test files on disk and remove the return types:

kevinquinnyo added a commit to kevinquinnyo/oauth2-client that referenced this pull request Jul 18, 2021
This adds two new interfaces:
- `ClockInterface` This is basically the spec from
https://github.com/php-fig/fig-standards/blob/master/proposed/clock.md
except without a return typehint to support PHP 5.6 for now
- `ClockAwareInterface` - this may not be necessary but it seemed
reasonable to have a `getClock(): ClockInterface` contract, given that
we expect to be able to get a clock in both `AccessToken` and
`AbstractProvider`.

This also removes some methods added in
thephpleague#852, specifically:
    - `AccessToken::setTimeNow()`
    - `AccessToken::getTimeNow()`
    - `AccessToken::resetTimeNow()`
    - `AccessToken::$timeNow`

That technically represents a BC break, but:
1. That code has not existed for very long and likely has very little
usage.
2. This *should* only break test code unless someone was doing something
very strange in production.

For the existing `AccessTokenInterface`:
    - remove the `setClock()` and `getTimeNow()`
        - `getTimeNow()` is no longer needed anyway
        - `setClock()` better not to have a setter on an interface in
        this case
        - both of these changes were BC breaks so they were removed

I believe the rest of the changes here are just cleaning up tests that
used the older static method of setting the current time. The previous
hack `tearDownForBackwardsCompatibility()` is no longer needed so it was
removed.
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