e2e Fix and Bump up#303
Conversation
vanshikaagupta22
force-pushed
the
windowsizefixAndBumpup
branch
from
0b99ec5 to
7ed5e6d
Compare
vanshikaagupta22
force-pushed
the
windowsizefixAndBumpup
branch
from
7ed5e6d to
51199f0
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the project version to 0.3.3-SNAPSHOT, bumps the selenium-chrome-driver to 4.13.0, and adjusts several timeout constants and Selenium configurations, including the use of the new headless mode. Feedback includes concerns regarding a version mismatch between Selenium dependencies that could lead to runtime errors, a logical inconsistency where the small timeout value now exceeds the medium timeout, and the potential for excessively high timeout settings to mask performance regressions.
| <groupId>org.seleniumhq.selenium</groupId> | ||
| <artifactId>selenium-chrome-driver</artifactId> | ||
| <version>4.11.0</version> | ||
| <version>4.13.0</version> |
There was a problem hiding this comment.
There is a significant version mismatch between selenium-chrome-driver (4.13.0) and selenium-java (4.0.0-alpha-1). Mixing such disparate versions of Selenium components is highly likely to cause runtime errors such as NoSuchMethodError due to breaking changes in the Selenium API. It is recommended to synchronize all Selenium dependencies to the same version.
| * SMALL_TIMEOUT_SECONDS: To be used as a small static wait (only if needed) | ||
| */ | ||
| public static final int SMALL_TIMEOUT_SECONDS = 5; | ||
| public static final int SMALL_TIMEOUT_SECONDS = 20; |
There was a problem hiding this comment.
The SMALL_TIMEOUT_SECONDS value (20) is now greater than the MEDIUM_TIMEOUT_SECONDS value (10), which creates a logical inconsistency in the naming of these constants. Please ensure that MEDIUM_TIMEOUT_SECONDS is also updated to a value greater than 20 to maintain a consistent hierarchy.
| public static final int DEFAULT_TIMEOUT_SECONDS = 300; | ||
| /** | ||
| * PAGE_LOAD_TIMEOUT_SECONDS: To be used as Selenium driver's default page load timeout | ||
| */ | ||
| public static final int PAGE_LOAD_TIMEOUT_SECONDS = 50; | ||
| public static final int PAGE_LOAD_TIMEOUT_SECONDS = 300; |
There was a problem hiding this comment.
Increasing the DEFAULT_TIMEOUT_SECONDS and PAGE_LOAD_TIMEOUT_SECONDS to 300 seconds (5 minutes) is quite high for E2E tests. While this may mitigate flakiness in slow environments, it can also lead to much longer build times when tests fail and may mask performance regressions. Consider if a more moderate value (e.g., 60-120 seconds) would be sufficient.
2 participants
No description provided.