Skip to content
This repository has been archived by the owner on Feb 13, 2020. It is now read-only.

Fix typehint of JobBuriedEvent to accept \Throwable #20

Merged

Conversation

vstm
Copy link
Contributor

@vstm vstm commented Nov 11, 2019

Otherwise this will result in an unrelated TypeError exception when an exception is thrown that is not inherited from \Exception.

This is related to PR #16

@@ -884,6 +884,29 @@ public function testFatalErrorCatch()

$this->assertEquals(false, $this->manager->executeJob($job));
}

public function testFatalErrorCatchWithNoRetries()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what this test adds in comparison to the previous test. I can see the difference but what fix does this test, and is it part of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your feeback - yes the difference between those two tests is subtle.

My test sets the releases attribute to 2 so the JobBuriedEvent is created. The previous test only reschedules the job and does not create the JobBuriedEvent. Maybe I can add an assertion that the JobBuriedEvent is dispatched to make this more explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have now updated the testcase to check if the buried event has actually been dispatched. I hope the intent of the test is now clearer @pkruithof

Otherwise this will result in an unrelated TypeError exception.
@vstm vstm force-pushed the fix/job-buried-event-typehint branch from dffd694 to f89629f Compare November 13, 2019 09:04
@pkruithof pkruithof merged commit 236139e into treehouselabs:master Nov 13, 2019
@pkruithof
Copy link
Contributor

Thank you @vstm!

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

Successfully merging this pull request may close these issues.

2 participants