pull request for neo test automation suite #14
Conversation
| // Wait for mini cart to appear | ||
| // Wait for the mini cart to show | ||
| miniCart.waitUntil(visible, Neodymium.configuration().selenideTimeout()); | ||
| miniCart.waitUntil(visible, 9000); |
There was a problem hiding this comment.
Here we should use values from our configuration, otherwise it will be a pain finding a specific piece of code to change values
There was a problem hiding this comment.
Discussed it, will not be handled now.
| // Wait for mini cart to disappear | ||
| // Wait for the mini cart to disappear | ||
| miniCart.waitUntil(not(visible), Neodymium.configuration().selenideTimeout()); | ||
| miniCart.waitUntil(not(visible), 9000); |
There was a problem hiding this comment.
Here we should use values from our configuration, otherwise it will be a pain finding a specific piece of code to change values
There was a problem hiding this comment.
Discussed it, will not be handled now.
There was a problem hiding this comment.
Here you used a lot of static data. This technical is possible, since it's stateless, but it's inconsistent to other components. The idea of the page/component concept is to have every component exactly accessible where it belongs.
Since search, minicart... etc. are part of the header you should put them to the header component and make them (only) accessible from there.
There was a problem hiding this comment.
fixed with commit "9d4d7d9bb148a3d2686a9f56660c5df5aa5dad6f" (changed static implementation)
| } | ||
|
|
||
| @Step("search for '{searchTerm}'") | ||
| @Step("search for {searchTerm}") |
There was a problem hiding this comment.
Since you have below methods which uses the search to either return a hits or no hits page, this should probably be private :)
There was a problem hiding this comment.
fixed with commit "9d4d7d9bb148a3d2686a9f56660c5df5aa5dad6f" (changed static implementation)
| @@ -13,48 +13,58 @@ | |||
|
|
|||
| public class Search extends AbstractComponent | |||
| { | |||
| private SelenideElement searchField = $("#s"); | |||
| private static SelenideElement searchField = $("#s"); | |||
There was a problem hiding this comment.
again please stick to the pageobject sheme and avoid statics
There was a problem hiding this comment.
fixed with commit "9d4d7d9bb148a3d2686a9f56660c5df5aa5dad6f" (changed static implementation)
| public abstract class AbstractTest | ||
| { | ||
| /// ----- dataobjects ----- /// | ||
|
|
||
| protected Address addressData; |
There was a problem hiding this comment.
Please have only the ones here which are shared between the tests. The others please put to the according class.
There was a problem hiding this comment.
fixed with commit "0f51495a5205b18f35da9489ea9ce1e79635f44d" (changed global testdata usage)
There was a problem hiding this comment.
To have a more streamline view: could you move this to a different package? Let's put all the "default user flow" tests like browsing, ordering, homepage into the smoke test package and the pagination and header tests in a feature, or component package
There was a problem hiding this comment.
fixed with commit "0f51495a5205b18f35da9489ea9ce1e79635f44d" (changed global testdata usage)
There was a problem hiding this comment.
fixed with commit "0f51495a5205b18f35da9489ea9ce1e79635f44d" (changed global testdata usage)
| OrderConfirmationPage.validateSuccessfulOrder(); | ||
| } | ||
|
|
||
| @After |
There was a problem hiding this comment.
What's the reason to remove this? Maybe to clarify the idea behind this. A failed test should not necessarily stop at a given point. For a registered user for example we might end up in a difficult situation.
If we (e.g.) fail during the billing step. The cart of the user will still be full and the next test run, which uses this user, will face an already filled cart on log in, which is not, what the tests expects.
Therefore the After method is trying to remove everything from the cart, regardless if tests was a success or not. If we succeeded the cart should be empty either way, if not, we need to clean up to ensure the start state of future tests.
There was a problem hiding this comment.
This applies everywhere where you might have removed the after method ;) Just in case I missed cases.
There was a problem hiding this comment.
fixed with commit "0f51495a5205b18f35da9489ea9ce1e79635f44d" (changed global testdata usage)
| * @author pfotenhauer | ||
| */ | ||
| //@Browser("Chrome_1024x768") | ||
| @Browser("Firefox_1024x768") |
There was a problem hiding this comment.
Is there any reason you removed the firefox? Since this is an example testsuite it would be good to showcase the possibility of having firefox added easily. (again applies to every place where you removed it ;) )
There was a problem hiding this comment.
fixed with commit "0f51495a5205b18f35da9489ea9ce1e79635f44d" (changed global testdata usage)
| @@ -1,19 +1,85 @@ | |||
| [ | |||
| { | |||
| "email": "[email protected]", | |||
There was a problem hiding this comment.
please use an @varmail.net address here (e.g. [email protected])
There was a problem hiding this comment.
fixed with commit "0f51495a5205b18f35da9489ea9ce1e79635f44d" (changed global testdata usage)
| { | ||
| "firstName": "John", | ||
| "lastName": "Doe", | ||
| "email": "[email protected]", |
There was a problem hiding this comment.
Use @varmail.de address
No description provided.