Mobile Emulation settings support for chromium-based browsers#158
Merged
Mobile Emulation settings support for chromium-based browsers#158
Conversation
+semver:feature
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntroduces an abstract ChromiumSettings to centralize Chromium browser option setup; ChromeSettings and EdgeSettings now extend it. Adds README mobile-emulation documentation, a test ScreenshotListener and BaseTest listener annotation, and CI steps to publish failure artifacts. No public API additions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/main/java/aquality/selenium/configuration/driversettings/ChromiumSettings.java (3)
29-35:getLoggingPreferences()is called twice; assign to a local variable.
getLoggingPreferences()triggers lazy initialisation on the first call and returns the cached field on the second. Both calls return the same object, so correctness is unaffected, but the double-access is a minor readability smell.♻️ Proposed fix
void setLoggingPreferences(MutableCapabilities options, String capabilityKey) { - if (!getLoggingPreferences().isEmpty()) { + Map<String, Level> prefs = getLoggingPreferences(); + if (!prefs.isEmpty()) { LoggingPreferences logs = new LoggingPreferences(); - getLoggingPreferences().forEach(logs::enable); + prefs.forEach(logs::enable); options.setCapability(capabilityKey, logs); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/aquality/selenium/configuration/driversettings/ChromiumSettings.java` around lines 29 - 35, The method setLoggingPreferences calls getLoggingPreferences() twice causing unnecessary repeated access and a readability smell; fix it by storing the result of getLoggingPreferences() into a local variable (e.g., var prefs = getLoggingPreferences()) then use prefs.isEmpty(), iterate prefs.forEach(logs::enable) and avoid the second call; update references inside setLoggingPreferences (MutableCapabilities options, String capabilityKey, LoggingPreferences logs) accordingly so behavior is unchanged but clearer.
71-73:setExperimentalOption("excludeSwitches", ...)is called unconditionally when there are no excluded arguments.When
getExcludedArguments()returns an empty list, passing it tosetExperimentalOption("excludeSwitches", [])explicitly sets an empty exclude-list on every session. This is semantically equivalent to not setting it, but adds unnecessary noise to the ChromeOptions and could interfere with any ChromeDriver default excluded-switch behaviour.♻️ Proposed fix
- protected <T extends ChromiumOptions<T>> void setExcludedArguments(T chromiumOptions) { - chromiumOptions.setExperimentalOption("excludeSwitches", getExcludedArguments()); - } + protected <T extends ChromiumOptions<T>> void setExcludedArguments(T chromiumOptions) { + List<String> excluded = getExcludedArguments(); + if (!excluded.isEmpty()) { + chromiumOptions.setExperimentalOption("excludeSwitches", excluded); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/aquality/selenium/configuration/driversettings/ChromiumSettings.java` around lines 71 - 73, The method setExcludedArguments currently calls chromiumOptions.setExperimentalOption("excludeSwitches", getExcludedArguments()) unconditionally; change it to first retrieve the list from getExcludedArguments() and only call chromiumOptions.setExperimentalOption("excludeSwitches", ...) when that list is non-empty (i.e., not null and not empty) so you don't set an explicit empty excludeSwitches array on every session; update the logic inside ChromiumSettings.setExcludedArguments to perform this guard using the getExcludedArguments() result.
57-68:setExperimentalOption("prefs", ...)is called unconditionally, even with an empty map.When
getBrowserOptions()returns an empty map (nooptionssection in settings.json),prefsis an emptyHashMapbutsetExperimentalOption("prefs", prefs)is still invoked. This injects an explicit but emptyprefsexperimental option into every Chrome/Edge session. While harmless today, it may conflict with a user-data-dir that already has prefs configured, or with future ChromeDriver stricter option validation.♻️ Proposed fix
- options.setExperimentalOption("prefs", prefs); + if (!prefs.isEmpty()) { + options.setExperimentalOption("prefs", prefs); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/aquality/selenium/configuration/driversettings/ChromiumSettings.java` around lines 57 - 68, The setPrefs method in ChromiumSettings unconditionally calls options.setExperimentalOption("prefs", prefs) even when prefs is empty; update ChromiumSettings.setPrefs to build prefs from getBrowserOptions() (mapping getDownloadDirCapabilityKey() to getDownloadDir()) as before but only call options.setExperimentalOption("prefs", prefs) if prefs is not empty (e.g., check prefs.isEmpty() and return/skip calling setExperimentalOption when empty) so you don't inject an empty "prefs" experimental option into Chrome/Edge sessions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 93-100: The README example is missing the documented "clientHints"
JSON that ChromiumSettings expects (CLIENT_HINTS_CAPABILITY_KEY) as a nested map
under mobileEmulation; update the example under "mobileEmulation" to include a
clientHints subsection showing typical keys (e.g., platform, mobile, etc.) so
users can see the correct structure that ChromiumSettings.parse/handling expects
when providing client-hints overrides.
In
`@src/main/java/aquality/selenium/configuration/driversettings/ChromiumSettings.java`:
- Around line 38-55: In setCapabilities(MutableCapabilities options) wrap the
result of getMapOrEmpty(...) into a new HashMap<>(...) before calling put to
ensure the map is mutable (replace assignments to mobileOptions with new
HashMap<>(getMapOrEmpty(...))). Also add an instanceof check for ChromiumOptions
before casting: if (!(options instanceof ChromiumOptions)) throw new
IllegalArgumentException("setCapabilities requires ChromiumOptions"); then
safely cast to (ChromiumOptions<?>) when calling setExperimentalOption with
MOBILE_EMULATION_CAPABILITY_KEY, DEVICE_METRICS_CAPABILITY_KEY and
CLIENT_HINTS_CAPABILITY_KEY.
---
Nitpick comments:
In
`@src/main/java/aquality/selenium/configuration/driversettings/ChromiumSettings.java`:
- Around line 29-35: The method setLoggingPreferences calls
getLoggingPreferences() twice causing unnecessary repeated access and a
readability smell; fix it by storing the result of getLoggingPreferences() into
a local variable (e.g., var prefs = getLoggingPreferences()) then use
prefs.isEmpty(), iterate prefs.forEach(logs::enable) and avoid the second call;
update references inside setLoggingPreferences (MutableCapabilities options,
String capabilityKey, LoggingPreferences logs) accordingly so behavior is
unchanged but clearer.
- Around line 71-73: The method setExcludedArguments currently calls
chromiumOptions.setExperimentalOption("excludeSwitches", getExcludedArguments())
unconditionally; change it to first retrieve the list from
getExcludedArguments() and only call
chromiumOptions.setExperimentalOption("excludeSwitches", ...) when that list is
non-empty (i.e., not null and not empty) so you don't set an explicit empty
excludeSwitches array on every session; update the logic inside
ChromiumSettings.setExcludedArguments to perform this guard using the
getExcludedArguments() result.
- Around line 57-68: The setPrefs method in ChromiumSettings unconditionally
calls options.setExperimentalOption("prefs", prefs) even when prefs is empty;
update ChromiumSettings.setPrefs to build prefs from getBrowserOptions()
(mapping getDownloadDirCapabilityKey() to getDownloadDir()) as before but only
call options.setExperimentalOption("prefs", prefs) if prefs is not empty (e.g.,
check prefs.isEmpty() and return/skip calling setExperimentalOption when empty)
so you don't inject an empty "prefs" experimental option into Chrome/Edge
sessions.
src/main/java/aquality/selenium/configuration/driversettings/ChromiumSettings.java
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/aquality/selenium/configuration/driversettings/ChromiumSettings.java (1)
50-50: Consider aninstanceofguard before casting toChromiumOptions<?>.
setCapabilitiesacceptsMutableCapabilities, so technically a non-Chromium options object could be passed. While the current call flow throughsetupDriverOptionsguarantees aChromiumOptions<T>, adding a defensive check would make the override more robust if the method is ever called directly.Suggested guard
- ((ChromiumOptions<?>)options).setExperimentalOption(key, mobileOptions); + if (!(options instanceof ChromiumOptions)) { + throw new IllegalArgumentException("Expected ChromiumOptions but got " + options.getClass().getName()); + } + ((ChromiumOptions<?>)options).setExperimentalOption(key, mobileOptions);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/aquality/selenium/configuration/driversettings/ChromiumSettings.java` at line 50, The cast to ((ChromiumOptions<?>) options) in ChromiumSettings.setExperimentalOption should be guarded with an instanceof check to avoid ClassCastException when a non-Chromium MutableCapabilities is passed; modify the code in ChromiumSettings (the method calling setExperimentalOption / using the options variable) to first verify options instanceof ChromiumOptions before casting and calling setExperimentalOption, and otherwise skip the experimental option or handle/log the unexpected type so the method is safe when invoked directly with non-Chromium MutableCapabilities.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@src/main/java/aquality/selenium/configuration/driversettings/ChromiumSettings.java`:
- Around line 38-55: In setCapabilities, getMapOrEmpty(...) may return an
immutable map causing mobileOptions.put(...) to throw; wrap the results in a new
HashMap to ensure mutability: replace the mobileOptions assignment with
Map<String,Object> mobileOptions = new
HashMap<>(getMapOrEmpty(getDriverSettingsPath(CapabilityType.CAPABILITIES.getKey(),
MOBILE_EMULATION_CAPABILITY_KEY))); and likewise wrap deviceMetrics and
clientHints (used for DEVICE_METRICS_CAPABILITY_KEY and
CLIENT_HINTS_CAPABILITY_KEY) with new HashMap<>(...) before putting them into
mobileOptions and passing to
((ChromiumOptions<?>)options).setExperimentalOption(...).
---
Nitpick comments:
In
`@src/main/java/aquality/selenium/configuration/driversettings/ChromiumSettings.java`:
- Line 50: The cast to ((ChromiumOptions<?>) options) in
ChromiumSettings.setExperimentalOption should be guarded with an instanceof
check to avoid ClassCastException when a non-Chromium MutableCapabilities is
passed; modify the code in ChromiumSettings (the method calling
setExperimentalOption / using the options variable) to first verify options
instanceof ChromiumOptions before casting and calling setExperimentalOption, and
otherwise skip the experimental option or handle/log the unexpected type so the
method is safe when invoked directly with non-Chromium MutableCapabilities.
Add ScreenshotListener for test failure analysis Collect failure artifacts in the pipeline
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/test/java/testreport/ScreenshotListener.java (2)
24-24: Redundant!result.isSuccess()check insideonTestFailure
onTestFailureis only invoked by TestNG when the test has already failed, so!result.isSuccess()is alwaystruehere.♻️ Proposed fix
- if(!result.isSuccess() && AqualityServices.isBrowserStarted()){ + if(AqualityServices.isBrowserStarted()){🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/testreport/ScreenshotListener.java` at line 24, In ScreenshotListener.onTestFailure remove the redundant condition using result.isSuccess() — since onTestFailure is only called for failed tests, replace the if(!result.isSuccess() && AqualityServices.isBrowserStarted()) check with a single check for AqualityServices.isBrowserStarted() (or invert to return early if the browser isn't started) so the failure-handling logic always runs when TestNG reports a failure.
29-29: Avoidorg.apache.logging.log4j.core.util.FileUtils— it's an internal log4j API
FileUtils.makeParentDirsis inlog4j-core'score.utilpackage, which is an internal utility not guaranteed to remain stable across minor/patch releases. Use the standardFiles.createDirectoriesinstead.♻️ Proposed fix
-import org.apache.logging.log4j.core.util.FileUtils; +import java.nio.file.Files; // already imported- FileUtils.makeParentDirs(destFile); + Files.createDirectories(destFile.toPath().getParent());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/testreport/ScreenshotListener.java` at line 29, Replace the use of the internal Log4j FileUtils in ScreenshotListener where FileUtils.makeParentDirs(destFile) is called: instead create the parent directories using java.nio.file API (e.g., obtain destFile.getParentFile().toPath() and call Files.createDirectories on it) and remove the dependency on org.apache.logging.log4j.core.util.FileUtils; ensure you handle or rethrow IOException consistently with the existing screenshot-saving logic in the same method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test/java/testreport/ScreenshotListener.java`:
- Line 21: In ScreenshotListener update the timestamp format to use 24-hour
hours by changing the SimpleDateFormat pattern used in the formatter from
"dd_MM_yyyy_hh_mm_ss" to "dd_MM_yyyy_HH_mm_ss" to avoid AM/PM collisions, and
when invoking Files.copy (the call that writes the screenshot file) add
StandardCopyOption.REPLACE_EXISTING as an option to prevent
FileAlreadyExistsException if a filename collision still occurs.
- Around line 25-34: The screenshot capture call is currently outside the try
block and can throw WebDriverException or ClassCastException from
getScreenshotAs / the unchecked cast; move the call to
((TakesScreenshot)AqualityServices.getBrowser().getDriver()).getScreenshotAs(OutputType.FILE)
into the existing try block in ScreenshotListener (or broaden the catch to
Exception) so that failures during screenshot capture are caught and logged by
the AqualityServices.getLogger().fatal handler instead of propagating; ensure
references to methodName and dateString remain used for destFile creation and
Reporter.log.
---
Nitpick comments:
In `@src/test/java/testreport/ScreenshotListener.java`:
- Line 24: In ScreenshotListener.onTestFailure remove the redundant condition
using result.isSuccess() — since onTestFailure is only called for failed tests,
replace the if(!result.isSuccess() && AqualityServices.isBrowserStarted()) check
with a single check for AqualityServices.isBrowserStarted() (or invert to return
early if the browser isn't started) so the failure-handling logic always runs
when TestNG reports a failure.
- Line 29: Replace the use of the internal Log4j FileUtils in ScreenshotListener
where FileUtils.makeParentDirs(destFile) is called: instead create the parent
directories using java.nio.file API (e.g., obtain
destFile.getParentFile().toPath() and call Files.createDirectories on it) and
remove the dependency on org.apache.logging.log4j.core.util.FileUtils; ensure
you handle or rethrow IOException consistently with the existing
screenshot-saving logic in the same method.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/java/testreport/ScreenshotListener.java (1)
19-21: Consider replacing legacyCalendar/SimpleDateFormatwithLocalDateTime/DateTimeFormatter
CalendarandSimpleDateFormatare legacy APIs;LocalDateTime+DateTimeFormatteris the idiomatic Java 8+ replacement and is also thread-safe by design.♻️ Proposed refactor
-import java.text.SimpleDateFormat; -import java.util.Calendar; +import java.time.LocalDateTime; +import java.time.format.DateTimeFormatter;- Calendar calendar = Calendar.getInstance(); - SimpleDateFormat formatter = new SimpleDateFormat("dd_MM_yyyy_HH_mm_ss"); - String dateString = formatter.format(calendar.getTime()); + String dateString = LocalDateTime.now().format(DateTimeFormatter.ofPattern("dd_MM_yyyy_HH_mm_ss"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/testreport/ScreenshotListener.java` around lines 19 - 21, Replace the legacy Calendar/SimpleDateFormat usage in ScreenshotListener with the Java 8 time API: use java.time.LocalDateTime.now() and a java.time.format.DateTimeFormatter.ofPattern("dd_MM_yyyy_HH_mm_ss") to produce dateString; update the variables where Calendar calendar, SimpleDateFormat formatter, and String dateString are declared/used to instead create a LocalDateTime now and call now.format(formatter), and add the necessary imports for LocalDateTime and DateTimeFormatter (removing Calendar and SimpleDateFormat imports).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/test/java/testreport/ScreenshotListener.java`:
- Line 29: The Files.copy call in ScreenshotListener.java can throw
FileAlreadyExistsException when retries/parameterized tests create the same dest
path; update the copy to use Files.copy(scrFile.toPath(), destFile.toPath(),
StandardCopyOption.REPLACE_EXISTING) (and add the StandardCopyOption import) so
existing files are replaced instead of failing silently, locating the change at
the Files.copy call in ScreenshotListener.
- Around line 25-31: In ScreenshotListener, expand the catch so the screenshot
failure doesn't propagate or hide the original test failure: catch
WebDriverException and ClassCastException in addition to IOException (e.g.,
catch (IOException | WebDriverException | ClassCastException e)) around the try
that calls ((TakesScreenshot)
AqualityServices.getBrowser().getDriver()).getScreenshotAs and the
Files.copy/Reporter.log lines; ensure you log the caught exception
(stacktrace/message) via process/test logging (Reporter.log or logger) so
failures to take a screenshot are recorded but do not replace the test's
original exception.
---
Nitpick comments:
In `@src/test/java/testreport/ScreenshotListener.java`:
- Around line 19-21: Replace the legacy Calendar/SimpleDateFormat usage in
ScreenshotListener with the Java 8 time API: use java.time.LocalDateTime.now()
and a java.time.format.DateTimeFormatter.ofPattern("dd_MM_yyyy_HH_mm_ss") to
produce dateString; update the variables where Calendar calendar,
SimpleDateFormat formatter, and String dateString are declared/used to instead
create a LocalDateTime now and call now.format(formatter), and add the necessary
imports for LocalDateTime and DateTimeFormatter (removing Calendar and
SimpleDateFormat imports).
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/java/forms/MyLocationForm.java (1)
19-22: Potential double-timeout wait on pages without a consent button.When
lblLatitudeis not yet displayed and the consent button is absent, the following sequence blocks:
btnConsent.state().waitForDisplayed()waits for the full configured timeout and returnsfalse.lblLatitude.state().waitForDisplayed()on line 22 then waits for another full timeout.This can make test failures take up to 2× the configured element timeout. If the consent button is only occasionally present (e.g., first visit, cookie consent), consider bounding the consent check to a short, dedicated timeout so the main wait on line 22 isn't delayed unnecessarily.
♻️ Proposed fix — use a short timeout for the consent check
- if (!lblLatitude.state().isDisplayed() && btnConsent.state().waitForDisplayed()) { + if (!lblLatitude.state().isDisplayed() && btnConsent.state().waitForDisplayed(java.time.Duration.ofSeconds(3))) { clickConsent(); } lblLatitude.state().waitForDisplayed();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/forms/MyLocationForm.java` around lines 19 - 22, The consent-check can block twice because btnConsent.state().waitForDisplayed() uses the full element timeout; change the consent probe to use a short, bounded timeout so you don't double-wait: replace the unconditional btnConsent.state().waitForDisplayed() call in the if condition with a short-timeout variant (e.g., state().waitForDisplayed(Duration.ofSeconds(1)) or the API-equivalent) and only call clickConsent() if that short wait returns true; then keep lblLatitude.state().waitForDisplayed() as the main full-timeout wait.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/test/java/forms/MyLocationForm.java`:
- Around line 19-22: The consent-check can block twice because
btnConsent.state().waitForDisplayed() uses the full element timeout; change the
consent probe to use a short, bounded timeout so you don't double-wait: replace
the unconditional btnConsent.state().waitForDisplayed() call in the if condition
with a short-timeout variant (e.g.,
state().waitForDisplayed(Duration.ofSeconds(1)) or the API-equivalent) and only
call clickConsent() if that short wait returns true; then keep
lblLatitude.state().waitForDisplayed() as the main full-timeout wait.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
+semver:feature