Allow custom requester + date match + use trait #56

pionl · 2020-06-15T20:10:12Z

Hi again, thank you for your package! I've modified the source to support few things we are currently using:

I'm unable to extend your class (Laravel TestCase) - I've added underlaying trait that will work the same
Added support for date match (ISO format)
I want to test our api schema without real HTTP connections - I've added ability to test Laravel kernel request -> https://github.com/pionl/laravel-swagger-test. A did not want to add any breaking changes and I've tried to minimize the implementation as possible.

I've not updated your documentation as my English is not so great, but do you want me to edit it?

Thank you for your time

froschdesign · 2020-06-16T06:10:17Z

src/AssertRequestAgainstSchema.php

+        // This code is only reached if the send is successful and
+        // all matches are satisfied. Otherwise an error is throwed before
+        // reach this
+        $this->assertTrue(true);


This is a wrong usage of a trait and typical mistake: this method is not defined in this trait and therefore it can not be used here. You have created a dependency which is not protected and leads to mistakes.

This will not work:

class Example { use \ByJG\ApiTools\AssertRequestAgainstSchema; public function foo() { $schema = \ByJG\ApiTools\Base\Schema::getInstance( file_get_contents('/path/to/json/definition') ); $this->setSchema($schema); $request = new \ByJG\ApiTools\ApiRequester(); $request ->withMethod('GET') ->withPath("/path/for/get/1"); $this->assertRequest($request); } }

It's not that bad. All that is missing is a line assert($this instanceof TestCase); at the beginning, plus a comment explaining that this is intended as plugin method for use in PHPUnit.

Or, another method could be to add an abstract declaration of assertTrue, though that might be problematic to do for the different PHP and PHPUnit versions. I'm not sure if this even works technically though, that a trait defines an abstract function that is then implemented by the baseclass of the class using the trait.

It's not that bad.

This has nothing to do with good or bad, it is wrong usage of a trait. Unfortunately you can see a lot of it in different libraries, frameworks and so on.

I'm not sure if this even works technically though, that a trait defines an abstract function that is then implemented by the baseclass of the class using the trait.

https://www.php.net/manual/en/language.oop5.traits.php#language.oop5.traits.abstract

Hi, not sure what you want me to do.

Comment that the method is expected

add abstract public static function assertTrue($condition, string $message = ''): void -- could cause problem with php version + phpunit as @UlrichEckhardt states.

Add check assert($this instanceof TestCase);

Of course, Trait will only work if Assert class is used.

Last change of assertTrue was 3 years ago with initial implementation :)

Of course, Trait will only work if Assert class is used.

Exactly that is the misuse of a trait and assert($this instanceof TestCase) is only a workaround which doesn't make things any better.

2. add abstract public static function assertTrue($condition, string $message = ''): void -- could cause problem with php version + phpunit as @UlrichEckhardt states.

Only with dead versions of PHP and PHPUnit. Bump the PHP version is an option here but this pull request is already mixing up too many topics. (imo)

Hi @froschdesign sorry for late response, lately I've noticed, that we do not need a the instance for assertTrue, we can use Assert::assertTrue! This will solve the not ideal "assertTrue" requirement. What do you think?

Sorry, I am late to the party, but I have to agree that refer a suppose parent class inside a trait it is not the best approach. However if Assert::assertTrue() doesn't need any instance, I believe you solve the issue.

UlrichEckhardt · 2020-06-16T06:35:55Z

src/ApiRequester.php

@@ -17,11 +20,31 @@ class ApiRequester extends AbstractRequester

    public function __construct()
    {
+        parent::__construct();


Nice, that's a fix I'd take.

UlrichEckhardt · 2020-06-16T06:39:12Z

src/Response/ResponseInterface.php

+    public function getStatusCode();
+
+    public function getBody();
+}


That's an interface without any documentation or guarantees describing what you can expect from it. I know you said you'd help documenting it, but it's hard to tell the intention with nothing.

Where would you expect the documentation to be? thank you

I've update the documentation. have a great day. Martin

UlrichEckhardt · 2020-06-16T06:45:38Z

src/Response/PsrResponse.php

+        return $this->interface->getBody();
+    }
+
+}


This is the only class implementing ResponseInterface, so I wonder why there is a separate interface at all. Also, it is just a wrapper around the existing interface, so it doesn't add any feature but just a layer of code.

Concerning this part of your proposed changes, I'd rather not merge them. BTW: I'm using this framework with Phalcon, which also doesn't have a PSR request and response type. In order to achieve that, we create the Phalcon type from the PSR type (simple conversion) and then convert the response in a similar way. We also use this in a way that no actual HTTP transfer is made, the request is directly injected into a kernel in-process. Is there a reason this doesn't work for you?

I understand, I was not able to do a simple "swap" of the requester, that's why I wanted separate layer to add ability to use "anything".

Here I'm converting Laravel response to interface that AbstractRequester needs:

https://github.com/pionl/laravel-swagger-test/blob/master/src/Response/LaravelResponse.php

Now I'm able to change handleRequest implementation and swap it with Laravel request:

https://github.com/pionl/laravel-swagger-test/blob/master/src/LaravelRequester.php

Of course I can build psr interface but BadResponseException will not be thrown (I would have to throw it -> more dependency on guzzle.

UlrichEckhardt · 2020-06-16T06:46:55Z

src/Base/Body.php

+     * @param $name
+     * @param $schema
+     * @param $body
+     * @param $type


I did not want to change the "code" style (I've copied the) same structure as matchString and etc. I can of course add correct types.

UlrichEckhardt · 2020-06-16T06:49:01Z

src/Base/Body.php

+            return null;
+        }
+
+        if (!(bool)strtotime($body)) {


Use strtotime($body) === false. The integer number zero is a non-error return value as per https://www.php.net/manual/en/function.strtotime.

That said, PHP does funny things with dates, like even parsing yesterday 12:00. I'm not sure about strtotime(), but I don't think this happens to match the OpenAPI standard by chance. What is the delta between this and the standard?

Ok, I've checked the specification and I've got it wrong - the date should not be implemented.

I should use

An optional format modifier serves as a hint at the contents and format of the string. OpenAPI defines the following built-in string formats: date – full-date notation as defined by RFC 3339, section 5.6, for example, 2017-07-21

At this moment matchString does not validate anything -- we could probably add a support for it?

So I've prepare format validation, but this could do a breaking change (tests fails because date-time has date string).

We can remove the matchDate and format validation and leave the PR only for custom request + trait

We can add the date format + future validations

pionl · 2020-06-16T10:10:08Z

I've update the code based on feedback - thank you for your time.

pionl · 2020-06-16T10:11:16Z

tests/OpenApiRequestBodyTest.php

+    {
+        // Missing Request
+        $datesTimes = [
+            '01-01T01:00:00+1200',


Should we add more invalid variants?

pionl · 2020-06-16T10:11:21Z

tests/OpenApiRequestBodyTest.php

+    {
+        // Missing Request
+        $datesTimes = [
+            '2000-01-01T01:00:00+1200',


Should we add more invalid variants?

byjg · 2020-07-05T15:58:17Z

I think these features are great, however some of them already implemented in the new version.

The CustomRequester feature was implemented by using the PSR7 (Web Request and URI) implementation. These implementation allows you have any implementation you want.

As example of Custom implementation we have the ApiRequester that actually executes a request to the api and the MockRequester that emulates the request by calling the class directly.

The other features (trait and date validator) I believe it is a good idea.

$requestBody parameter can be also array

https://swagger.io/docs/specification/data-models/oneof-anyof-allof-not/

By default we can extend provided test case, for situations where we can't extend the test case we can use trait AssertRequestAgainstSchema.

To allow custom requester that is not making a http request (like laravel's test cases)

pionl · 2020-11-11T10:02:45Z

Ok guys, I'll split the implmentation, change the the laravel requester and etc and send a new PR. Thank you

froschdesign reviewed Jun 16, 2020

View reviewed changes

UlrichEckhardt reviewed Jun 16, 2020

View reviewed changes

pionl commented Jun 16, 2020

View reviewed changes

tests/OpenApiRequestBodyTest.php

{

// Missing Request

$datesTimes = [

'01-01T01:00:00+1200',

Copy link

Author

pionl Jun 16, 2020

Choose a reason for hiding this comment

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

Should we add more invalid variants?

pionl commented Jun 16, 2020

View reviewed changes

filda and others added 2 commits September 17, 2020 16:24

Update AbstractRequester.php

6a095e5

$requestBody parameter can be also array

Add basic support for additional properties settings

4300039

pionl force-pushed the allow-custom-requester branch from 08b594e to f3e0b4b Compare September 17, 2020 14:44

pionl added 8 commits September 25, 2020 11:08

Check if the string type value is string

70695c9

Add support for allOf

3316c06

https://swagger.io/docs/specification/data-models/oneof-anyof-allof-not/

Add trait for asserting schema requests

5843004

By default we can extend provided test case, for situations where we can't extend the test case we can use trait AssertRequestAgainstSchema.

Make AbstractRequester more abstract for custom non-psr clients

b425193

To allow custom requester that is not making a http request (like laravel's test cases)

Support date schema match

53f820c

Use correct date format validation

a2f17be

Update ReponseInterface documentation

149ea6d

Check if the trait is used in PHPUnits TestCase

482db43

pionl force-pushed the allow-custom-requester branch from f3e0b4b to 482db43 Compare September 25, 2020 09:23

jakubdibala and others added 2 commits January 20, 2021 09:21

Do not fail if not found in path and try to match every path defined

d8fade8

Use Guzzle v7

8f2f8d2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow custom requester + date match + use trait #56

Allow custom requester + date match + use trait #56

pionl commented Jun 15, 2020

froschdesign Jun 16, 2020

UlrichEckhardt Jun 16, 2020

UlrichEckhardt Jun 16, 2020

froschdesign Jun 16, 2020 •

edited

Loading

pionl Jun 16, 2020

pionl Jun 16, 2020

pionl Jun 16, 2020

froschdesign Jun 16, 2020

pionl Jun 27, 2020

byjg Jul 5, 2020 •

edited

Loading

UlrichEckhardt Jun 16, 2020

UlrichEckhardt Jun 16, 2020

pionl Jun 16, 2020

pionl Jun 27, 2020

UlrichEckhardt Jun 16, 2020

pionl Jun 16, 2020 •

edited

Loading

pionl Jun 16, 2020

UlrichEckhardt Jun 16, 2020

pionl Jun 16, 2020

UlrichEckhardt Jun 16, 2020

pionl Jun 16, 2020

pionl Jun 16, 2020

pionl commented Jun 16, 2020

pionl Jun 16, 2020

pionl Jun 16, 2020

byjg commented Jul 5, 2020

pionl commented Nov 11, 2020

Allow custom requester + date match + use trait #56

Are you sure you want to change the base?

Allow custom requester + date match + use trait #56

Conversation

pionl commented Jun 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

froschdesign Jun 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byjg Jul 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pionl Jun 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pionl commented Jun 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byjg commented Jul 5, 2020

pionl commented Nov 11, 2020

froschdesign Jun 16, 2020 •

edited

Loading

byjg Jul 5, 2020 •

edited

Loading

pionl Jun 16, 2020 •

edited

Loading