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 the system date to be mocked in PHPUnit tests. #6407

Closed
techanvil opened this issue Jan 13, 2023 · 6 comments
Closed

Allow the system date to be mocked in PHPUnit tests. #6407

techanvil opened this issue Jan 13, 2023 · 6 comments
Labels
P2 Low priority Type: Infrastructure Engineering infrastructure & tooling

Comments

@techanvil
Copy link
Collaborator

techanvil commented Jan 13, 2023

Feature Description

We should provide the ability to mock the system date in PHPUnit tests, in order to be able to write more robust tests which use the date.

This is following up on this comment thread, and the related Analytics 4 test covering the default date range parameters should be rewritten with the mock date solution.

Additionally the codebase should be checked for any other PHPUnit tests which are using the system date and these should also be rewritten with mocked dates.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • It should be possible to mock the system date in PHPUnit tests.
  • Existing tests which make use of the system date should be updated to use mocked dates.

Implementation Brief

  • The approach would be to mock global time-related functions within specific namespaces, ensuring function lookups within these namespaces use the mocked version instead of the global ones.
  • Functions to Mock:
    • gmdate
    • strtotime
  • Targeted Namespaces:
    • Google\Site_Kit\Core\Util
    • Google\Site_Kit\Modules\Analytics_4\Report
    • Google\Site_Kit\Modules
  • Create a new class that can mock the time() function based on the date set in the tests.
  • Overridden functions in the specified namespaces will refer to this class to retrieve the modified date/time.
  • Refer to this PoC PR for technical implementation.

Test Coverage

  • Only date in tests/phpunit/integration/Modules/Analytics_4Test.php should be replaced with mocked method, no other test needs updating

QA Brief

Changelog entry

@techanvil techanvil added P2 Low priority Type: Infrastructure Engineering infrastructure & tooling labels Jan 13, 2023
@tofumatt tofumatt self-assigned this Jan 13, 2023
@tofumatt
Copy link
Collaborator

ACs here are good, moving to IB 👍🏻

@aaemnnosttv
Copy link
Collaborator

@techanvil do we know if this is possible? We can of course introduce a mockable abstraction around time or other functions for our use but that's not the same as controlling the output of \time() itself, which would be necessary to achieve this given the integration with WP which uses the global/built-in functions.

@zutigrm zutigrm self-assigned this Oct 24, 2023
@zutigrm zutigrm mentioned this issue Oct 25, 2023
18 tasks
@zutigrm zutigrm removed their assignment Oct 25, 2023
@tofumatt tofumatt self-assigned this Nov 7, 2023
@tofumatt
Copy link
Collaborator

tofumatt commented Nov 7, 2023

@zutigrm The proposed solution doesn't seem to mock the main \time() function, which as @aaemnnosttv pointed out is used by WordPress/other plugins. For this to be implemented we would need to be able to intercept all calls to time() and other date/time related functions.

Is that possible to do? If not we should probably close this issue. At first I thought we could use namespaces (like under Google\Site_Kit\Tests or something) to mock time(), but of course it would only mock our calls, not everything 🤔

@tofumatt tofumatt assigned zutigrm and unassigned tofumatt Nov 7, 2023
@zutigrm
Copy link
Collaborator

zutigrm commented Nov 7, 2023

@tofumatt Thank you for the feedback. Yes correct, this will not mock the system time, the proposed change is targeting the 3 namespaces that use date/time functions, so this would mock the report time mainly (which seems to be the main need for why this issue was raised, unless I am getting that wrong).

Mocking the system time, might be possible but we would need to bring in new testing library, that might help with that, just to achieve system time mock (although it might not guarantee the success). Not sure this would be recommended to do?
cc: @aaemnnosttv

@aaemnnosttv
Copy link
Collaborator

It looks like we could redefine internal functions using runkit functions like runkit7_function_redefine (which we use in a few places within tests) although I'm not sure if it provides a way to restore the original implementation.

Otherwise, I've used Carbon in the past as an expressive wrapper around DateTimes which also allows for setting the current test time which is really what we want to do here. If we're only looking to mock it to control our own use of time functions, we could refactor our implementation to use this instead.

For things where WP's use of these functions is involved, I think changing our approach to the assertions should be all we need, as in #6758.

@aaemnnosttv
Copy link
Collaborator

Closing this for now as there isn't a way to mock the system time that would also affect WP without tinkering with core PHP functions which I think we should avoid (at least for now).

If we do decide to use something like Carbon in the future, that would be great but would be a better fit for a different issue.

Aside from that, I think we can update our approach to some issues where we've seen fragility in our tests as mentioned above.

@aaemnnosttv aaemnnosttv closed this as not planned Won't fix, can't repro, duplicate, stale Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 Low priority Type: Infrastructure Engineering infrastructure & tooling
Projects
None yet
Development

No branches or pull requests

4 participants