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

Added Behat Tests for WebService #24162

Merged
merged 1 commit into from Oct 27, 2021
Merged

Conversation

Progi1984
Copy link
Contributor

@Progi1984 Progi1984 commented Apr 22, 2021

Questions Answers
Branch? develop
Description? Added Basic Tests for WebService
Type? improvement
Category? TE
BC breaks? no
Deprecations? no
Fixed ticket? Relative to #23100
How to test? N/A
Possible impacts? N/A

This change is Reviewable

@Progi1984 Progi1984 requested a review from a team as a code owner April 22, 2021 14:33
@prestonBot prestonBot added develop Branch Improvement Type: Improvement labels Apr 22, 2021
@KminekMatej
Copy link
Contributor

Just what I needed to start with testing Webservice, thanks!

Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

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

Hi !

Whao ! that is a great improvement 😊

May I suggest 2 things? I would like to know what you think about it

  1. Use Behat to introduce reader-friendly assertion steps
    For example we could wrap
        self::assertEquals(1, $output->filter('prestashop > errors > error')->count());

inside a nice Behat step

THEN I expect 1 error

or

self::assertEquals(
            'No permission for this authentication key',
            $output->filter('prestashop > errors > error')->first()->filter('message')->text()
        );

by

THEN I expect the error message 'No permission for this authentication key' to be returned
  1. I think webservice can return XML or JSON. You use the DOMCrawler to navigate into the output XML if I understand correctly.

If using the DOMCrawler makes the writing of the tests harder, maybe using JSON output will be easier? However I am not sure Webservice allows JSON input.

We could also think about enabling Webservice to return something else than JSON and XML such as a serialized PHP object. It would then become a lot easier to assert and you would not need to parse the output. But that requires changing the implementation of the webservice and this could be complex and risky.

classes/webservice/WebserviceKey.php Outdated Show resolved Hide resolved
);

// Add an object in database
$data = '<prestashop xmlns:xlink="http://www.w3.org/1999/xlink">
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to avoid repeating these custom XML document when you add more tests, maybe a function generateInputXMLForAddress() could be used?

@KminekMatej
Copy link
Contributor

@matks Prestashop currently supports output as XML OR JSON, and both of the outputs are being generated by different classes. So there should be one test for XML output (using DOMcrawler) and second test for JSON output (not done yet).

Webservice definitely doesnt allow JSON input, it takes only XML. Would be nice to have this feature in the future, as JSON is currently considered as standard for API.

About Webservice returning serialized object. You already have Webservice, capable of returning JSON or XML output. I really wouldn't recommend adding another output type, such as serialized object, since those two are more than enough to cover all customers needs. You would have just more code to maintain, not mentioning that JSON output is a standard nowadays and its super easy to decode /encode it into any structure of basically any language.

@matks
Copy link
Contributor

matks commented Apr 23, 2021

@matks Prestashop currently supports output as XML OR JSON, and both of the outputs are being generated by different classes. So there should be one test for XML output (using DOMcrawler) and second test for JSON output (not done yet).

You can workaround this issue by splitting the Webservice in component.

Consider that the Webservice is a system that outputs data in a dedicated format. We could say it's built on 3 components

  1. A component that fetches the data from database
  2. A component that gathers, reworks, refines the data extracted from database like an ETL
  3. A component that outputs the built data in wanted format

When you send a HTTP request to webservice

  • component 1 does its job, outputs some raw data from MySQL
  • component 2 does its job, transform raw data into structured data
  • component 3 does its job, transform structured data into wanted format (JSON or XML)

If you are able to test very well each component, you do not need a full test coverage of all possible outputs for JSON and XML. If you can use tests to validate that component 2 works very well alone, then the tests for component 3 only need to verify "when I give it a data structure like A it should output format B". Component 3 is an encoder (see https://developer.apple.com/documentation/foundation/jsonencoder for example) and could actually be replaced by available encoders for PHP.

Nevertheless it would be important to have a few tests that run components 1, 2 and 3 together to make sure they are plugged to one another correctly 😄 . But you do not need to be exhaustive with this last set of tests.

So when I suggest to output PHP serialized objects, the idea behind (sorry if I did not explain it correctly) is "let's isolate components 2 and components 3, use a PHP object structure between the 2 of them as their way to communicate, and when we want to test component 2 we just need to serialize whatever comes out of it instead of performing encoding".

That, however, is a big work. I was just curious to see if @Progi1984 is interested in this for the future.

@KminekMatej
Copy link
Contributor

KminekMatej commented Apr 23, 2021

@matks Idea of splitting the Webservice into components is very good. That is actually exactly what I developed in my awaiting PR #23100 . There I am composing component 2 using PHP class ApiNode. After I compose all the structure in the ApiNode tree, I use either WebServiceOutputJSON or WebServiceOutputXML components to transform that tree structure into wanted format.

However, during autotesting, you should simulate real usage as much as you can. Process is, that user sends request, where he specifies wanted output format (using IO-Format header, json/xml). Output that Webservice gives to him is already in wanted format. In my opinion, you should simulate this principle as much as you can, thats why you need to create two test, where you give as input some data and test if the XML answer is proper. Afterwards, you should test the same operation goes identically, also with JSON output (these two tests can share the same initial phase, no redundant code needed).

I dont actually think, as you stated, that you are able to test very well each component. You can test the whole process at once. And I believe thats how you should test this.

@matks
Copy link
Contributor

matks commented Apr 23, 2021

I dont actually think, as you stated, that you are able to test very well each component. You can test the whole process at once. And I believe thats how you should test this.

Each type of test brings value, but usually the low-level tests

  • are easier to write
  • catch issues at a deeper level
  • are faster to run

You can see the great article The Test Pyramid
from Martin Fowler about it

  • Write tests with different granularity
  • The more high-level you get the fewer tests you should have

Stick to the pyramid shape to come up with a healthy, fast and maintainable test suite: Write lots of small and fast unit tests. Write some more coarse-grained tests and very few high-level tests that test your application from end to end. Watch out that you don't end up with a test ice-cream cone that will be a nightmare to maintain and takes way too long to run.

Copy link
Contributor

@sowbiba sowbiba left a comment

Choose a reason for hiding this comment

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

Nice Job @Progi1984

tests/Integration/Webservice/AbstractWebserviceTest.php Outdated Show resolved Hide resolved
tests/Integration/Webservice/AbstractWebserviceTest.php Outdated Show resolved Hide resolved
tests/Integration/Webservice/AddressesTest.php Outdated Show resolved Hide resolved
tests/Integration/Webservice/AddressesTest.php Outdated Show resolved Hide resolved
@KminekMatej
Copy link
Contributor

+1 for merge, I am ready to create additional tests over WebService afterwards

@KminekMatej
Copy link
Contributor

Is there any progress in merging this so far?

@Progi1984 Progi1984 changed the title Added Basic Tests for WebService Added Behat Tests for WebService Oct 1, 2021
@Progi1984 Progi1984 requested a review from matks October 1, 2021 14:50
Copy link
Contributor

@jolelievre jolelievre left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @Progi1984
It's really good that we start testing this part! However I see some flows in the previous implementation, that you used as a base for the new context I guess

basically relying on stateful stored data in behat tests is a bad practice, we already have lots of pain to test our code because of our steaful contexts, if we start adding some in the tests themselves we will soon start fighting with a three headed hydra 😅

It's not always easy for complex cases I know that, but I made a few suggestions which, I believe, will allow us to get rid of stateful test contexts

@Progi1984 Progi1984 added the Waiting for author Status: action required, waiting for author feedback label Oct 9, 2021
@Progi1984 Progi1984 removed the Waiting for author Status: action required, waiting for author feedback label Oct 14, 2021
Copy link
Contributor

@jolelievre jolelievre left a comment

Choose a reason for hiding this comment

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

Thanks @Progi1984
It's much better now, I added a few comments and questions because I didn't fully understand everything

@Progi1984 Progi1984 added the Waiting for author Status: action required, waiting for author feedback label Oct 21, 2021
@Progi1984 Progi1984 removed the Waiting for author Status: action required, waiting for author feedback label Oct 26, 2021
PierreRambaud
PierreRambaud previously approved these changes Oct 26, 2021
@atomiix atomiix added the Waiting for QA Status: action required, waiting for test feedback label Oct 27, 2021
@PierreRambaud PierreRambaud removed the Waiting for QA Status: action required, waiting for test feedback label Oct 27, 2021
Copy link
Contributor

@jolelievre jolelievre left a comment

Choose a reason for hiding this comment

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

Thank you @Progi1984 all good for me too

Sorry for being such a pain on this 😅. I still think it is now more readable, usable, and stable I hope you share this feeling ^^

Scenario: Test if Bad WS Key
When I use Webservice with key "ABCDEINVALIDDDDDDDD" to list "addresses"
Then I should get 1 error
And I should get an error with code 18 and message "Invalid authentication key format"
Copy link
Contributor

Choose a reason for hiding this comment

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

I could be picky on this one and suggest using some humans readable strings instead of hard coded int code here
But I've already been enough of a PITA for you on this PR so I will stop here 😅

@jolelievre jolelievre merged commit cb9bee6 into PrestaShop:develop Oct 27, 2021
@Progi1984 Progi1984 deleted the issue23100 branch October 27, 2021 13:22
@Progi1984
Copy link
Contributor Author

@KminekMatej 🎉 It's a good news :)

@KminekMatej
Copy link
Contributor

@Progi1984 Such a great job! That completely unlocks my PR with API core refactoring. Thanks!

@Progi1984
Copy link
Contributor Author

@KminekMatej Thank you for waiting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
develop Branch Improvement Type: Improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants