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

[5.4] Change how frequencies are tested for schedule #17086

Merged
merged 4 commits into from
Jan 3, 2017
Merged

[5.4] Change how frequencies are tested for schedule #17086

merged 4 commits into from
Jan 3, 2017

Conversation

alexbowers
Copy link
Contributor

A change to how schedules are tested for their expression.

I opted to not use mocks, since Cache is not required for the tests to happen.

This, unfortunately, requries PHP 7.0+ to be done in this way, so if you wish to have support for the 5.6+, then this will be required:

$this->event = new Event(
    m::mock('Illuminate\Contracts\Cache\Repository'),
    'php foo'
);

If #17084 gets accepted, then this test is required:

/**
 * @test
 */
function every_x_minutes()
{
    $this->assertEquals("*/6 * * * * *", $this->event->everyXMinutes(6)->getExpression());
}

If #17085 gets accepted, then this test is required:

/**
 * @test
 */
function weekends()
{
    $this->assertEquals("* * * * 0,6 *", $this->event->weekends()->getExpression());
}

@alexbowers alexbowers changed the title Change how frequencies are tested for schedule [5.4] Change how frequencies are tested for schedule Jan 2, 2017
/**
* @test
*/
function every_minute()
Copy link
Member

Choose a reason for hiding this comment

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

this does't match the style of our other tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, correct you are. I'll change them to match the format.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. Not that there's anything wrong with that format, it looks pretty nice, but, just not for laravel/frameworks tests please. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, its my macro for creating tests. I use it for my personal tests, and out of habit used them here. My mistake, should have noticed that :)

@alexbowers
Copy link
Contributor Author

@taylorotwell
Copy link
Member

So these new tests require PHP 7?

@alexbowers
Copy link
Contributor Author

@taylorotwell Not anymore. The original commit did, and I would strongly like to alter how the test works in Laravel 5.5+ when PHP 7.0+ is a requirement to eliminate an unnecessary mock, but for now, it works in PHP 5.6+

@alexbowers
Copy link
Contributor Author

There is a test written for the weekends that just got merged in too, I can apply that test tomorrow to this PR.

@taylorotwell taylorotwell merged commit bee1ad2 into laravel:5.4 Jan 3, 2017
@alexbowers
Copy link
Contributor Author

@taylorotwell Please can you add in the test in my first message above for weekends, or should I submit it as a separate PR?

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