NEW: Mock sleep unit test utility.

[mfendeksilverstripe]

NEW: Mock sleep unit test utility.

Simple utility function for unit tests. Use instead of sleep(). I found this quite handy is several projects, we might as well make the code reusable.

[GuySartorelli]

I don't think we should be including methods that are only meant to be called in tests into production codebases.
There is some of that already, but it's too late to decouple those instances in the current major. I don't think we should be adding more.

I'll leave this open for a day or two in case a core committer disagrees with me, though.

[mfendeksilverstripe]

@GuySartorelli Fair enough, is there a better place for it though? For example we could add a TestOnly service maybe under the Dev namespace?

[GuySartorelli]

I'm not sure, to be honest. I'd love (in the long term) for all of the test-related stuff to be moved out of framework altogether, and be in its own module that can be included as a dev dependency... but we're a ways off doing something like that just yet.

This might be the best place for it at the moment, to be honest, now that I've spent a bit more time considering it.
I still won't merge immediately in case there are opinions from other core committers on this but yeah probably this is the spot for this method, if there is a spot for it, in the current state of things.

[mfendeksilverstripe]

All good @GuySartorelli , happy to keep the PR open. Having a separate module for this sounds great.

[michalkleiner]

Fair consideration from Guy, I would agree here. We can wait forever or just get it in now and move it out later with other stuff.

[michalkleiner]

Let's wait a day or two if Max or Steve have a different view, and if not we can get it in.

[emteknetnz]

This does seem like the wrong place

Somewhere in src/Dev seems like it would be better. There are already some other Test*.php classes there such as TestSession and TestMailer

So perhaps create a new class Dev/TestDateTime.php and put this method it there?

[dhensby]

Couldn't it be added to SapphireTest or some part of the PHPUnit framework to allow tests to call $this->addTime() or similar?

How would one expect this to work if mock_now was not set?

[mfendeksilverstripe]

@dhensby Move the feature to new location as requested, please review. In case time mock is not set it will fallback to current time which I think is fine.

public static function now()
{
    $time = self::$mock_now ? self::$mock_now->Value : time();

    /** @var DBDatetime $now */
    $now = DBField::create_field('Datetime', $time);

    return $now;
}

[GuySartorelli]

@mfendeksilverstripe Looks like you added the method twice?

[mfendeksilverstripe]

@GuySartorelli That's because SapphireTest has two classes in there (PHPUnit 5 and 9), so I covered both.

[GuySartorelli]

Ahh yup, fair point. I forgot about that.

[emteknetnz]

I'd be inclined to just delete the PHPUnit 5.7 version, it's an ancient API we shouldn't be developing for it. Everyone is encouraged to migrate off it. Unit tests in CI only get tested against PHPUnit 9

What happens if you put a sleep(3) in your unit test?

[GuySartorelli]

I'd be inclined to just delete the PHPUnit 5.7 version,

Just for the sake of clarity: I assume you mean the 5.7 version of the new method, not of the class itself.

[dhensby]

Cool. Is the time reset after each test? I assume it is, just making sure...

[GuySartorelli]

If it isn't, that's a fundamental issue with the mock now functionality as a whole, not a new problem introduced with this PR.

[dhensby]

Right, but a dev who implements mock_now is then responsible for the tear down of that test. This feature hides it behind syntactic sugar, so it'd be understandable that a dev that uses it assumes the date is reset at the end of the test for them.

[dhensby]

Looks like it is handled: https://github.com/silverstripe/silverstripe-framework/blob/4/src/Dev/SapphireTest.php#L630-L632

[GuySartorelli]

@emteknetnz You've still got a review here with requested changes - is that for removing the new method on the old phpunit class? I don't think it's worth holding up merging for that personally, are you okay with the PR as is or do you still want some changes made?

[emteknetnz]

I'm not too attached, feel free to merge if you see fit