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

AbstractRequesterTest: Add a test #53

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

UlrichEckhardt
Copy link
Contributor

This is a very basic unit test of the AbstractRequesterClass. I've been experimenting with the handling of query parameters (see master branch here for a work in progress snapshot) and found that no such test exists. All other tests always use this class in a much wider context, but not in isolation.

Also, there's an oddity which I stumbled across: The path segment /v1 is neither present in the code making the request nor is it in the code handling the request. Still, things seem to work fine.

@UlrichEckhardt UlrichEckhardt force-pushed the abstract-requester-tests branch 2 times, most recently from 4693a7d to 208ebd3 Compare April 27, 2020 06:13
@byjg
Copy link
Owner

byjg commented Apr 27, 2020

You're completely right. The class dont have a test case for query 🤦

I'd love to implement this feature, but as you know I have a upcoming version 3.1.x-dev (PR #50) with a huge internal refactory, including the ability to mock the request against the schema. And for coincidence I already created a test class in this new version with the same name you created.

Take a look specifically here:
https://github.com/byjg/php-swagger-test/blob/559e0c5f38d5ff2159f71dbbca5f6ec8d996227e/tests/AbstractRequesterTest.php

I can see two options:

  1. accept this PR to master. But I'll have to refactor a lot to accomodate with the new code when a merge the version 3.1. My plan is to release in mid-May.
  2. If you can adjust this tests to the new version and change the PR to point to the branch 3.1.

What do you think?

@UlrichEckhardt
Copy link
Contributor Author

I'll rebase it on 3.1, shouldn't be too big of a challenge. My main goal was to establish a base for further tests. If you happened to do that already, I'm perfectly fine with it.

BTW: The tests for my feature branch failed for PHP 7.4 due to a deprecation notice. Reason was an incompatibility between 7.4 and the PHPUnit installed there. It would help if you cherry-picked 05ccd9f, just to get this already out of the way.

Happy hacking and muito obrigado! :)

@byjg
Copy link
Owner

byjg commented Apr 27, 2020

OK, I cherry-picked the commit you indicate into 3.1 branch.

Muito obrigado for your help here :)

@UlrichEckhardt
Copy link
Contributor Author

I think this can be closed, as the tests you wrote are even more than the single one I wrote. I do think that you complicated this a bit though. PHPUnit comes with a whole suite of tools for mocking things. All you need to do is create a mock for AbstractRequester where the (abstract) handleRequest() method is mocked. You can then set up this method to both validate what it receives and flexibly return whatever response you want.

I also noticed an actual error in your code. Check out withPath() in AbstractRequester. It calls withPath on the PSR7 URI class. However, those two work very differently! The second one is documented per PSR7 with "This method MUST retain the state of the current instance, and return an instance that contains the specified path.", so it will not modify the object you call it on and your data will not be stored because the returnvalue is discarded. Of course, the tests seem to show the opposite, but that's because your PSR7 implementation is flawed and does not treat objects as immutable.

@byjg
Copy link
Owner

byjg commented Apr 30, 2020

First of all, thanks for the considerations you did. I really appreciate.

Now, let's divide in two parts your comments.

The first one is "Why I am not using the PHPUnit Mock?" The answer is because there are differents tests to be tested :) If I am just testing the AbstractRequester implementation makes sense mock it because I want to test the single class unit by removing the things are not deterministic. However the primary intended use of this component is to provide a suite to:

  • Validate if the specification is what I expected to be
  • Run a Functional test case and check if the parameters I am sending and response are in fact what I expected
  • Instead of check with conditionals if the parameters my method is receiving is correct, I can use this component to validate against the OpenApi specification
  • Function test case without really do a real request, and that is the reason why I have the AbstractRequester. I am doing an implementation using another component to put all this things togheter. This component is restserver and it is an implementation of a Rest Server using the OpenApi specification.

Since I already have the MockRequester as an example to fake a request I prefered to use it in the tests. However it doest'n exclude use the PHPUnit Mock for test the component and it is welcome.

About the immutable implementation you are 100% right and I just miss it. Thank you for the heads up. I followed your recommendation and implemented the Uri component immutable and adapt the Message, Request and Response PSR7 implementation I have in the WebRequest component to follow the same rule as specified in the documentation. I already updated the PHP Swagger Test to support this new implementation. All tests passed :)

@froschdesign
Copy link

@UlrichEckhardt
Good catch! 👍

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