TestNG template project - reviewed by free CodeRabbit with GPT-4-turbo model #40
Conversation
|
WalkthroughThe update involves refactoring and enhancing a Selenium-based test automation framework. It introduces new classes for better modularity, updates dependencies, and improves the initialization of browser modules. The changes aim to streamline the testing process with TestNG, improve logging with Allure, and facilitate data-driven tests using JSON. Changes
Uplevel your code reviews with CodeRabbit ProCodeRabbit ProIf you like this project, please support us by purchasing the Pro version. The Pro version has advanced context, superior noise reduction and several proprietary improvements compared to the open source version. Moreover, CodeRabbit Pro is free for open source projects. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Files selected (15)
- aquality-selenium-template-cucumber/src/test/java/aquality/selenium/template/cucumber/hooks/AqualityServicesHooks.java (1)
- aquality-selenium-template-cucumber/src/test/java/aquality/selenium/template/cucumber/objectfactory/CustomBrowserModule.java (1)
- aquality-selenium-template-cucumber/src/test/java/aquality/selenium/template/cucumber/objectfactory/CustomObjectFactory.java (2)
- aquality-selenium-template-testng/pom.xml (1)
- aquality-selenium-template-testng/src/test/java/aquality/selenium/template/testng/steps/ui/ContactUsSteps.java (1)
- aquality-selenium-template-testng/src/test/java/aquality/selenium/template/testng/steps/ui/LandingSteps.java (1)
- aquality-selenium-template-testng/src/test/java/aquality/selenium/template/testng/tests/BaseTest.java (1)
- aquality-selenium-template-testng/src/test/java/aquality/selenium/template/testng/tests/ContactUsTests.java (1)
- aquality-selenium-template-testng/src/test/java/aquality/selenium/template/testng/utilities/JsonDataProvider.java (1)
- aquality-selenium-template-testng/src/test/java/aquality/selenium/template/testng/utilities/ModuleFactory.java (1)
- aquality-selenium-template/src/main/java/aquality/selenium/template/modules/CustomBrowserModule.java (1)
- aquality-selenium-template/src/main/java/aquality/selenium/template/modules/ServiceModule.java (1)
- aquality-selenium-template/src/main/java/aquality/selenium/template/utilities/AllureBasedLocalizedLogger.java (3)
- aquality-selenium-template/src/main/java/aquality/selenium/template/utilities/CustomActionRetrier.java (1)
- pom.xml (1)
Files ignored due to filter (1)
- aquality-selenium-template-testng/src/test/resources/data/contacts.json
Files skipped from review due to trivial changes (2)
- aquality-selenium-template-cucumber/src/test/java/aquality/selenium/template/cucumber/hooks/AqualityServicesHooks.java
- pom.xml
Review comments generated (35)
- Review: 21
- LGTM: 14
Tips
Chat with 
CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
| @@ -14,7 +15,7 @@ public class CustomObjectFactory implements ObjectFactory { | |||
|
|
|||
| public CustomObjectFactory() { | |||
| this.injector = Guice.createInjector(Stage.PRODUCTION, CucumberModules.createScenarioModule(), | |||
| new ServiceModule(), new CustomBrowserModule(AqualityServices::getBrowser)); | |||
| new ServiceModule(), new CustomBrowserModule()); | |||
There was a problem hiding this comment.
The change in the CustomObjectFactory constructor removes the reference to AqualityServices::getBrowser when creating a new CustomBrowserModule. This could potentially alter the behavior of how the browser is managed within the tests. Ensure that the CustomBrowserModule is correctly implemented without the need for this reference, and that it does not introduce any regressions or unexpected behaviors.
- new ServiceModule(), new CustomBrowserModule(AqualityServices::getBrowser));
+ new ServiceModule(), new CustomBrowserModule());| @Step("Main page is opened") | ||
| public void checkMainPageIsOpened() { | ||
| assertThat(mainPage.state().waitForDisplayed()) | ||
| .describedAs(mainPage.getName() + " must be opened") | ||
| .isTrue(); |
There was a problem hiding this comment.
The method checkMainPageIsOpened uses an assertion to check if the main page is displayed. However, it does not handle the case where the assertion fails and the test continues running, potentially leading to misleading test results. It would be better to throw an exception if the condition is not met, ensuring that the test stops execution at this point.
- assertThat(mainPage.state().waitForDisplayed())
- .describedAs(mainPage.getName() + " must be opened")
- .isTrue();
+ boolean isDisplayed = mainPage.state().waitForDisplayed();
+ if (!isDisplayed) {
+ throw new AssertionError(mainPage.getName() + " must be opened");
+ }| <?xml version="1.0" encoding="UTF-8"?> | ||
| <project xmlns="http://maven.apache.org/POM/4.0.0" | ||
| xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
| xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> | ||
| <modelVersion>4.0.0</modelVersion> | ||
| <parent> | ||
| <groupId>com.github.aquality-automation</groupId> | ||
| <artifactId>aquality-selenium-template-project</artifactId> | ||
| <version>1.0.1-SNAPSHOT</version> | ||
| </parent> | ||
|
|
||
| <artifactId>aquality-selenium-template-testng</artifactId> | ||
|
|
||
| <properties> | ||
| <maven.compiler.source>11</maven.compiler.source> | ||
| <maven.compiler.target>11</maven.compiler.target> | ||
| <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | ||
|
|
||
| </properties> | ||
|
|
||
| <dependencies> | ||
| <dependency> | ||
| <groupId>com.github.aquality-automation</groupId> | ||
| <artifactId>aquality-selenium-template</artifactId> | ||
| <version>1.0.1-SNAPSHOT</version> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.testng</groupId> | ||
| <artifactId>testng</artifactId> | ||
| <version>7.9.0</version> | ||
| <scope>test</scope> | ||
| <exclusions> | ||
| <exclusion> | ||
| <groupId>org.slf4j</groupId> | ||
| <artifactId>slf4j-api</artifactId> | ||
| </exclusion> | ||
| </exclusions> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>io.qameta.allure</groupId> | ||
| <artifactId>allure-testng</artifactId> | ||
| <version>${allure.version}</version> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.assertj</groupId> | ||
| <artifactId>assertj-core</artifactId> | ||
| <version>3.25.3</version> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.slf4j</groupId> | ||
| <artifactId>slf4j-simple</artifactId> | ||
| <version>2.0.10</version> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| </dependencies> | ||
| </project> |
There was a problem hiding this comment.
The Maven POM file for the aquality-selenium-template-testng project is well-structured and includes necessary dependencies for a TestNG-based Selenium project. However, there are a couple of points that need attention:
- The
${allure.version}property is used in line 43 but it's not defined within the<properties>section. This could lead to a build failure if the property is not defined elsewhere. - There is no newline at the end of the file (line 59). While this is not a functional issue, it is a common practice to include a newline at the end of files to comply with POSIX standards.
<properties>
<maven.compiler.source>11</maven.compiler.source>
<maven.compiler.target>11</maven.compiler.target>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
+ <allure.version>2.13.9</allure.version> <!-- Example version, should be replaced with the actual version intended to use -->
</properties>
...
</dependencies>
</project>
+ <!-- Newline added at the end of the file -->| import aquality.selenium.template.utilities.IScreenshotProvider; | ||
| import com.google.inject.Inject; |
There was a problem hiding this comment.
The use of IScreenshotProvider and dependency injection with @Inject is good for maintainability and modularity, but ensure that the implementation of IScreenshotProvider is thread-safe if tests are to be run in parallel.
| @Guice(moduleFactory = ModuleFactory.class) | ||
| public class BaseTest { |
There was a problem hiding this comment.
Using the @Guice annotation with a custom module factory is a good practice for dependency injection. It allows for better test configuration and easier maintenance.
| @Test(description = "Empty email validation is working on Contact Us page", | ||
| dataProvider = "getContacts", dataProviderClass = JsonDataProvider.class) | ||
| public void testEmailValidation(ContactUsInfo contactInfo) { | ||
| landingSteps.checkMainPageIsOpened(); | ||
| landingSteps.openContactUsPage(); | ||
| contactUsSteps.checkContactUsPageIsOpened(); | ||
|
|
||
| contactUsSteps.saveContactUsPageDump(); | ||
| contactUsSteps.fillContactUsPage(contactInfo); | ||
| contactUsSteps.acceptPrivacyAndCookiesPolicy(); | ||
| contactUsSteps.clickSendButton(); | ||
|
|
||
| contactUsSteps.checkEmptyFieldWarning(); | ||
| contactUsSteps.contactUsPageDumpIsDifferent(); | ||
| } |
There was a problem hiding this comment.
The test method testEmailValidation seems to be testing multiple behaviors: navigation, form submission, and validation messages. Consider breaking this down into smaller tests that each verify a single behavior for better maintainability and clarity. Additionally, the method name suggests it only tests email validation, but the steps indicate broader functionality is being tested.
- @Test(description = "Empty email validation is working on Contact Us page",
+ @Test(description = "Navigate to Contact Us page")
public void navigateToContactUsPage() {
landingSteps.checkMainPageIsOpened();
landingSteps.openContactUsPage();
contactUsSteps.checkContactUsPageIsOpened();
}
+ @Test(description = "Check empty email field warning on Contact Us page",
+ dependsOnMethods = {"navigateToContactUsPage"})
public void checkEmptyEmailFieldWarning(ContactUsInfo contactInfo) {
contactUsSteps.saveContactUsPageDump();
contactUsSteps.fillContactUsPage(contactInfo);
contactUsSteps.acceptPrivacyAndCookiesPolicy();
contactUsSteps.clickSendButton();
contactUsSteps.checkEmptyFieldWarning();
contactUsSteps.contactUsPageDumpIsDifferent();
}| contactUsSteps.saveContactUsPageDump(); | ||
| contactUsSteps.fillContactUsPage(contactInfo); | ||
| contactUsSteps.acceptPrivacyAndCookiesPolicy(); | ||
| contactUsSteps.clickSendButton(); | ||
|
|
||
| contactUsSteps.checkEmptyFieldWarning(); | ||
| contactUsSteps.contactUsPageDumpIsDifferent(); |
There was a problem hiding this comment.
The method saveContactUsPageDump and contactUsPageDumpIsDifferent suggest that there is some kind of comparison being made between the state of the page before and after an action. This could potentially be flaky if the page content is dynamic or if there are timing issues. Ensure that these methods are reliable and consider using more deterministic checks where possible.
- contactUsSteps.saveContactUsPageDump();
- contactUsSteps.fillContactUsPage(contactInfo);
- contactUsSteps.acceptPrivacyAndCookiesPolicy();
- contactUsSteps.clickSendButton();
-
- contactUsSteps.checkEmptyFieldWarning();
- contactUsSteps.contactUsPageDumpIsDifferent();
+ // Ensure that the page dump comparison logic is robust against dynamic content and timing issues.
+ // Consider using explicit waits or assertions that target specific elements or states.| @UtilityClass | ||
| public class JsonDataProvider { | ||
| private static final ObjectMapper MAPPER = new ObjectMapper(); | ||
|
|
||
| @DataProvider | ||
| @SneakyThrows | ||
| public Object[][] getContacts() { | ||
| return getDataListFromJson("contacts", ContactUsInfo[].class); | ||
| } | ||
|
|
||
| private Object[][] getDataListFromJson(String filename, Class<? extends Object[]> valueType) { | ||
| Object[] dataList; | ||
| try { | ||
| File file = Paths.get(String.format("src/test/resources/data/%s.json", filename)).toFile(); | ||
| dataList = MAPPER.readValue(file, valueType); | ||
| } | ||
| catch (IOException exception) { | ||
| AqualityServices.getLogger().debug("IO error occurred while reading the data from the file " + filename, exception); | ||
| throw new RuntimeException(exception); | ||
| } | ||
| return new Object[][] {dataList}; | ||
| } |
There was a problem hiding this comment.
The JsonDataProvider class is using a UtilityClass annotation from Lombok, which is intended for classes that only have static methods and fields. However, the use of this annotation along with TestNG's @DataProvider annotation could be misleading because data providers are typically not static in nature as they are meant to be invoked by the testing framework. Additionally, there is an exception handling issue where a generic RuntimeException is thrown after catching an IOException. It would be better to throw a more specific custom exception that provides context about the failure related to test data provisioning.
- @UtilityClass
- public class JsonDataProvider {
+ public final class JsonDataProvider {
+ private JsonDataProvider() {
+ // Private constructor to prevent instantiation
+ }
- throw new RuntimeException(exception);
+ throw new TestDataReadException("IO error occurred while reading the data from the file " + filename, exception);
+ }
+
+ public static class TestDataReadException extends RuntimeException {
+ public TestDataReadException(String message, Throwable cause) {
+ super(message, cause);
+ }
+ }| @DataProvider | ||
| @SneakyThrows | ||
| public Object[][] getContacts() { | ||
| return getDataListFromJson("contacts", ContactUsInfo[].class); | ||
| } |
There was a problem hiding this comment.
Using @SneakyThrows to bypass checked exceptions can lead to unexpected runtime failures without proper handling. It's generally better to handle checked exceptions explicitly or declare them in the method signature so that users of the method are aware of potential exceptions.
- @SneakyThrows
- public Object[][] getContacts() {
+ public Object[][] getContacts() throws IOException {
return getDataListFromJson("contacts", ContactUsInfo[].class);
}| private Object[][] getDataListFromJson(String filename, Class<? extends Object[]> valueType) { | ||
| Object[] dataList; | ||
| try { | ||
| File file = Paths.get(String.format("src/test/resources/data/%s.json", filename)).toFile(); | ||
| dataList = MAPPER.readValue(file, valueType); | ||
| } | ||
| catch (IOException exception) { | ||
| AqualityServices.getLogger().debug("IO error occurred while reading the data from the file " + filename, exception); | ||
| throw new RuntimeException(exception); | ||
| } | ||
| return new Object[][] {dataList}; |
There was a problem hiding this comment.
The method getDataListFromJson reads a JSON file and converts it into an array of objects. The current implementation assumes that the JSON file will always be correctly formatted and located in the expected directory. It would be beneficial to add checks to ensure that the file exists before attempting to read it, and to provide a clear error message if the file is not found.
+ if (!file.exists()) {
+ throw new TestDataReadException("Test data file not found: " + file.getAbsolutePath());
+ }
dataList = MAPPER.readValue(file, valueType);Additionally, wrapping the entire array within another array (new Object[][] {dataList}) may not be necessary depending on how the data provider is used in the tests. If each element of dataList is supposed to be passed as a separate set of parameters to a test method, then you should iterate over dataList and create a two-dimensional array where each sub-array contains one element from dataList.
- return new Object[][] {dataList};
+ Object[][] dataProvider = new Object[dataList.length][];
+ for (int i = 0; i < dataList.length; i++) {
+ dataProvider[i] = new Object[] {dataList[i]};
+ }
+ return dataProvider;
|
mialeska
changed the title
Summary by CodeRabbit
CustomBrowserModuleandCustomActionRetrierto enhance browser interactions and retry mechanisms.ServiceModuleto improve modularity.AllureBasedLocalizedLoggerfor better logging capabilities integrated with Allure reports.ContactUsSteps,LandingSteps,BaseTest,ContactUsTestsclasses for streamlined UI testing workflows.JsonDataProviderandModuleFactoryutilities for data-driven testing and module management in TestNG tests.