Conversation
There was a problem hiding this comment.
Pull request overview
Updates the Selenium TestNG headless setup to reduce bot-detection failures in CI (Chrome/Chromedriver), while also adjusting a few UI test interactions/selectors and removing an unused load-test class.
Changes:
- Removed
SeleniumLoadTest(ad-hoc threaded load test). - Expanded
HeadlessBaseTestChromeOptions + CDP masking, added retry startup and richer debugging artifacts, and updated several waits/interactions. - Updated page objects/selectors used by “Unlock offers” and cart offer flows.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/Test/SeleniumLoadTest.java | Removed the standalone load-test runner. |
| src/test/java/Test/HeadlessBaseTest.java | Major headless Chrome configuration changes (anti-bot masking, retry logic, debug output) plus multiple test flow wait updates and failure artifact capture. |
| src/test/java/Test/BaseTest.java | Updates selectors/flows for unlock-offers and cart-offers scenarios; removes an unused import. |
| src/test/java/Pages/UnlockOffers.java | Adds brand/color filter interactions and updates the shoe selector used by unlock-offers tests. |
| src/test/java/Pages/CartOffers.java | Makes the sports-bra selector less brittle by matching a prefix. |
| src/test/java/Pages/CartCheckout.java | Adjusts driver field visibility/constructor placement; leaves commented code. |
Comments suppressed due to low confidence (1)
src/test/java/Test/HeadlessBaseTest.java:209
- This
@BeforeMethodtakes and stores a screenshot even on successful runs. For larger suites this can be expensive and flood CI artifacts; prefer capturing screenshots only on failure (you already do this inteardown) or when bot-block is detected.
try {
java.io.File screenshot = ((org.openqa.selenium.TakesScreenshot) driver)
.getScreenshotAs(org.openqa.selenium.OutputType.FILE);
java.nio.file.Files.copy(screenshot.toPath(),
new java.io.File("target/debug-homepage-" + System.currentTimeMillis() + ".png").toPath(),
java.nio.file.StandardCopyOption.REPLACE_EXISTING);
System.out.println("Screenshot saved to target/ directory");
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| By colorfilter = By.xpath("//div[normalize-space()='Color']"); | ||
| public void clickColorFilter() { | ||
| driver.findElement(colorfilter).click(); | ||
| } |
There was a problem hiding this comment.
UnlockOffers now has both colorfilter (new) and colourfilter (existing) locators/methods for the same UI concept. This duplication is confusing and easy to drift; consolidate to a single locator/method and use consistent naming.
| // Enable JavaScript (some sites check) | ||
| options.addArguments("--enable-javascript"); |
There was a problem hiding this comment.
--enable-javascript is not a standard Chrome command-line switch (JavaScript is enabled by default). Passing unknown/invalid flags can cause noisy warnings or, in some environments, startup failures; remove this flag (or only use --disable-javascript when you explicitly want it off).
| options.addArguments("--disable-web-security"); | ||
|
|
||
| // The "Masking" kit from BaseTest | ||
| options.setExperimentalOption("excludeSwitches", java.util.Collections.singletonList("enable-automation")); | ||
| options.addArguments("--allow-running-insecure-content"); |
There was a problem hiding this comment.
Disabling web security / allowing insecure content (--disable-web-security, --allow-running-insecure-content) significantly changes browser behavior and can mask real issues (CORS/mixed-content) in UI flows. If this is only needed for specific anti-bot experiments, gate it behind an env flag and keep the default secure configuration.
| java.nio.file.Files.writeString( | ||
| java.nio.file.Path.of("target/debug-page-source-" + System.currentTimeMillis() + ".html"), | ||
| pageSource | ||
| ); | ||
| System.out.println("Page source saved to target/ directory"); |
There was a problem hiding this comment.
This @BeforeMethod writes the full page source to target/ unconditionally. That can bloat CI artifacts and slow runs; consider writing it only when possibleBotBlock is true or when a debug env flag is enabled.
| // wait.until(ExpectedConditions.elementToBeClickable(By.xpath("(//div[@class='cart-offer-callout-card-header'])[1]"))); //(//div[@class='cart-offer-callout-card-header'])[1] | ||
| // unlockOffers.isBestOfferTextDisplayed("Best Offer - Apply at Checkout"); |
There was a problem hiding this comment.
Leaving commented-out waits/assertions in a test makes it unclear what the intended verification is and reduces coverage for the offers modal content. Either restore an assertion that verifies the expected text/state, or remove the dead commented code and replace with a stable check.
| // WebDriver driver; | ||
|
|
There was a problem hiding this comment.
Please remove the commented-out // WebDriver driver; line; commented code in page objects adds noise and makes it unclear which driver field should be used.
| options.addArguments("--window-size=1920,1080"); | ||
| options.addArguments("--disable-popup-blocking"); |
There was a problem hiding this comment.
--window-size=1920,1080 sets the viewport at Chrome startup, but later the test also calls driver.manage().window().setSize(...). Having both is redundant and can lead to confusion about which size is actually in effect; prefer keeping only one approach.
| // Accept languages/encoding to look like a real browser | ||
| options.addArguments("--lang=en-US,en;q=0.9"); | ||
| options.addArguments("--accept-encoding=gzip, deflate, br"); |
There was a problem hiding this comment.
--accept-encoding=... is an HTTP header concept, not a supported Chrome startup argument, and the value here includes spaces. This is likely ignored or treated as an unknown flag; drop it and, if you need to control headers, set them via CDP/DevTools on requests instead.
| wait.until(ExpectedConditions.visibilityOfElementLocated(By.xpath("(//div[@class='product-discount-additional'])[1]"))); | ||
| unlockOffers.isAdditonalOffersSectionDisplayed("Additional offers for you"); |
There was a problem hiding this comment.
isAdditonalOffersSectionDisplayed(...) returns a boolean but the result is ignored, so this step doesn't actually validate anything. Convert this into an assertion (or remove the helper call if it’s not meant to assert).
Made changes for the chromedriver to isolate antibote detection