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

Change the implementation to use only a trait. #20

Merged
merged 2 commits into from
Mar 25, 2020
Merged

Change the implementation to use only a trait. #20

merged 2 commits into from
Mar 25, 2020

Conversation

sebastianbergmann
Copy link
Contributor

This removes the ProphecyTestCase class and moves the remaining logic to the ProphecyTrait trait.

@stof
Copy link
Member

stof commented Mar 25, 2020

@sebastianbergmann I added assertions on the status of tests in #21 to ensure that a failed prediction is properly reported as a failure and not as an error. This is passing with the existing implementation and PHPUnit before sebastianbergmann/phpunit@0c85ee1 (tests are of course failing after this commit as we cannot override verifyMockObjects anymore).
When I run these new assertions with your new implementation, testMockPredictionFailure fails because it reports things as error. It looks like teardown methods cannot report a test failure anymore, even if they perform extra assertions. We may need a new @verify hook instead running earlier.

@sebastianbergmann
Copy link
Contributor Author

sebastianbergmann commented Mar 25, 2020

Good catch! I tested this locally, of course, and it worked. But that was obviously without #21.

The correct template method to use would be assertPostConditions(). This method is called between the test method and tearDown().

There is no annotation support for assertPreConditions() and assertPostConditions() yet, but it should be easy to add support for @preConditions and @postConditions for PHPUnit 9.1. Before I implement @preConditions and @postConditions, though, we should check whether @postConditions would actually solve our problem.

Can you rename checkProphecyPredictions to assertPostConditions and remove the @after annotation, please, and test whether that works? Thanks!

@stof
Copy link
Member

stof commented Mar 25, 2020

Well, changing this to assertPostConditions fixed the testMockPredictionFailure but broke testSpyPredictionFailure

@stof
Copy link
Member

stof commented Mar 25, 2020

OK, for testSpyPredictionFailure, this might be solved by reverting the removal of prophecyTearDown done in your tests. IIRC, this method is meant to fixed this case.

@stof
Copy link
Member

stof commented Mar 25, 2020

I confirm that re-introducing prophecyTearDown and using assertPostConditions makes the testsuite green.

@stof
Copy link
Member

stof commented Mar 25, 2020

@sebastianbergmann I merged #21 so the new assertions are now part of the testsuite.

@stof
Copy link
Member

stof commented Mar 25, 2020

Note however that I really like the integration based purely on a trait rather than needing a base class. It makes things much easier.

@sebastianbergmann
Copy link
Contributor Author

Note however that I really like the integration based purely on a trait rather than needing a base class. It makes things much easier.

As do I: which is why this pull request removes ProphecyTestCase.

@stof
Copy link
Member

stof commented Mar 25, 2020

@sebastianbergmann could you rebase your own master branch on top of the upstream one rather than having done the opposite (which does not actually fix merge conflicts, and duplicates the commit from #21) ?

Btw, it might be easier to use feature branches to send PRs rather than using your master branch.

@sebastianbergmann
Copy link
Contributor Author

Done.

@stof stof changed the title Simplify Change the implementation to use only a trait. Mar 25, 2020
@stof stof merged commit 386ea46 into phpspec:master Mar 25, 2020
@stof
Copy link
Member

stof commented Mar 25, 2020

@sebastianbergmann the diff in sebastianbergmann/phpunit#4141 (comment) about the removal of the built-in Prophecy support contains 2 removals which might affect this third-party integration:

  • the removal of the error conversion just before calling onNotSuccessfulTest (which still gets applied when using this prophecy-phpunit package).
  • the removal of catching PredictionException to mark the status as failure (it would then probably be catched by the following block and mark it as error).

It might be great to plan for a replacement feature, to allow these things to be done from the trait. I fear that the testsuite of prophecy-phpunit would not stay green with such removal patch applied.

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.

2 participants