feat: Develop actions for mobile - #10
Conversation
dakusui
left a comment
There was a problem hiding this comment.
Review summary
This PR adds an Appium-based actions/mobile package mirroring the existing Playwright actions/web package, plus the io.appium:java-client:10.1.1 dependency and unit tests. The dependency itself checks out (version exists on Maven Central; no Selenium version conflict with this repo).
The main theme of the findings: the port is a surface copy of the web package — several load-bearing design properties of the web version didn't survive the translation. Inline comments below, most severe first:
PageFunctionscannot feed the acts — every method returnsFunction<AppiumDriver, WebElement>, butClick/ClickIfPresent/SendKeyaccept onlyFunction<AppiumDriver, By>; also resolves eagerly, unlike the lazy webLocatordesign. (PageFunctions.java)- XPath built by string concatenation breaks on single quotes in all six locator builders. (PageFunctions.java)
ClickIfPresentdrops the web version's visibility guard — clicks on mere presence. (ClickIfPresent.java)Files.copymay hit a missing parent directory — the framework doesn't reliably pre-create the test-result dir for the main stage. (Screenshot.java)MASK_PREFIXredefined whileInternalUtils.mask()is bound to the web constant — the two can drift. (SendKey.java)- No
name()override onSendKey— action trees lose the target locator. (SendKey.java) linkLocatorByTextbuilds byte-identical XPath tolocatorByTextand doesn't restrict to link-like elements despite its Javadoc. (PageFunctions.java)- Screenshot: per-call temp-file leak + double write, and a
Path→String→File→Pathround-trip. (Screenshot.java)
Minor (no inline comments): PageFunctions/ElementFunctions use 4-space indent vs the 2-space house style; ElementFunctions declares a redundant explicit private constructor in an enum; several new files lack trailing newlines; MASK_PREFIX's doc comment is a truncated sentence ("A prefix to control a").
Bottom line: the act classes themselves are reasonable, but findings 1–2 make PageFunctions effectively unusable as shipped. I'd suggest the Function<AppiumDriver, By> redesign plus the visibility/directory fixes before merge.
🤖 Generated with Claude Code
| /// @param name A name (accessibility id / content-desc) of a link-like element. | ||
| /// @return A function that resolves a locator specified by `name` in a given `AppiumDriver` object. | ||
| /// | ||
| public static Function<AppiumDriver, WebElement> linkLocatorByName(String name) { |
There was a problem hiding this comment.
[1] Type incompatibility: PageFunctions cannot feed the acts in this package.
Every method here returns Function<AppiumDriver, WebElement>, but every mobile act constructor (Click, ClickIfPresent, SendKey) accepts only Function<AppiumDriver, By>. In the web package the two halves share one currency — new ClickIfPresent(PageFunctions.locatorByText("hello")) compiles and is the documented usage (see BuiltInActsTest.java:83) — but the mobile equivalent is a compile error, so this whole utility class is unreachable from the acts it was written for. The new tests don't catch this because they exercise the two halves in isolation and never compose them.
Related: these functions call d.findElement(...) eagerly, so applying one when the element is absent throws NoSuchElementException at apply-time, instead of returning a lazy, retriable handle like the web Locator.
Suggestion — fix both at once: return Function<AppiumDriver, By> (build the By.xpath(...) lazily and let the act perform the single findElement). That restores the single-currency design and the lazy semantics, and avoids double element resolution.
There was a problem hiding this comment.
Follow-up with a concrete reproduction, verified against this PR's head (398ae96).
The branch itself compiles because nothing in the PR ever passes a PageFunctions result into an act — the new tests exercise each half in isolation. The incompatibility surfaces on the first attempt to compose them the way the web package documents and tests (cf. new ClickIfPresent(PageFunctions.locatorByText("hello")) in BuiltInActsTest). Adding this file:
package jp.co.moneyforward.autotest.ut.builtins;
import jp.co.moneyforward.autotest.actions.mobile.Click;
import jp.co.moneyforward.autotest.actions.mobile.ClickIfPresent;
import jp.co.moneyforward.autotest.actions.mobile.PageFunctions;
import jp.co.moneyforward.autotest.actions.mobile.SendKey;
/// Mobile transplant of compositions the web package supports and tests.
class MobileCompositionRepro {
void composeActsWithPageFunctions() {
new Click(PageFunctions.buttonLocatorByName("Submit"));
new ClickIfPresent(PageFunctions.locatorByText("hello"));
new SendKey(PageFunctions.locatorByPlaceholder("Enter name"), "text");
}
}and running mvn test-compile fails on all three lines:
[ERROR] MobileCompositionRepro.java:[17,5] no suitable constructor found for Click(Function<AppiumDriver,WebElement>)
[ERROR] constructor Click(Function<AppiumDriver,By>) is not applicable
[ERROR] (argument mismatch; Function<AppiumDriver,WebElement> cannot be converted to Function<AppiumDriver,By>)
[ERROR] MobileCompositionRepro.java:[18,5] no suitable constructor found for ClickIfPresent(Function<AppiumDriver,WebElement>)
[ERROR] MobileCompositionRepro.java:[19,5] no suitable constructor found for SendKey(Function<AppiumDriver,WebElement>,String)
So every PageFunctions method is currently unreachable from every act in the package.
Suggested guard once the signatures are aligned (mirrors the existing web test, and would have turned this into a red build):
@Test
void givenMobilePageFunctionsLocator_whenConstructingActs_thenComposable() {
AppiumDriver driver = Mockito.mock(AppiumDriver.class);
ExecutionEnvironment env = Mockito.mock(ExecutionEnvironment.class);
WebElement element = Mockito.mock(WebElement.class);
when(driver.findElement(any(By.class))).thenReturn(element);
when(driver.findElements(any(By.class))).thenReturn(List.of(element));
// The point of this test is that these constructor calls COMPILE,
// i.e. PageFunctions' return type is the acts' input type.
new jp.co.moneyforward.autotest.actions.mobile.Click(PageFunctions.buttonLocatorByName("Submit")).perform(driver, env);
new jp.co.moneyforward.autotest.actions.mobile.ClickIfPresent(PageFunctions.locatorByText("hello")).perform(driver, env);
new jp.co.moneyforward.autotest.actions.mobile.SendKey(PageFunctions.locatorByPlaceholder("Enter name"), "text").perform(driver, env);
Mockito.verify(element, Mockito.atLeast(2)).click();
Mockito.verify(element).sendKeys("text");
}🤖 Generated with Claude Code
| /// | ||
| public static Function<AppiumDriver, WebElement> linkLocatorByName(String name, boolean lenient) { | ||
| String xpath = lenient | ||
| ? "//*[contains(@content-desc,'" + name + "') or contains(@name,'" + name + "')]" |
There was a problem hiding this comment.
[2] XPath built by raw string concatenation breaks on any argument containing a single quote.
linkLocatorByName("O'Brien") produces //*[@content-desc='O'Brien' or @name='O'Brien'] → InvalidSelectorException at findElement. This affects all six locator builders in this class (linkLocatorByName, locatorByText, buttonLocatorByName, locatorByLabel, locatorByPlaceholder, linkLocatorByText). The web version is immune by construction (getByText/getByRole take the value as data, not markup).
Suggestion: prefer AppiumBy.accessibilityId(...) where applicable (it maps to content-desc on Android and name on iOS automatically, replacing the hand-maintained OR-xpaths), or route user text through a single shared XPath-literal quoting helper (concat('...', "'", '...') technique).
There was a problem hiding this comment.
これ、一回踏むと、デバッグつらそうなので、直した方がいいかも。(Fableとも相談した)
|
|
||
| @Override | ||
| public AppiumDriver perform(AppiumDriver driver, ExecutionEnvironment executionEnvironment) { | ||
| List<WebElement> elements = driver.findElements(this.locatorFunction.apply(driver)); |
There was a problem hiding this comment.
[3] Visibility guard dropped relative to the web ClickIfPresent.
The web version clicks only if targetElement.isVisible() (its Javadoc: "Check for presence is done by Locator#isVisible"). This copy clicks whenever findElements() returns a non-empty list, so an element that is present in the tree but not visible (off-screen, hidden view) gets clicked — or throws ElementNotInteractableException — which is exactly the "skip safely" case this class exists for.
Suggestion:
if (!elements.isEmpty() && elements.getFirst().isDisplayed()) {
elements.getFirst().click();
}| public AppiumDriver perform(AppiumDriver value, ExecutionEnvironment executionEnvironment) { | ||
| File screenshot = value.getScreenshotAs(OutputType.FILE); | ||
| try { | ||
| Files.copy(screenshot.toPath(), |
There was a problem hiding this comment.
[4] Files.copy does not create the destination's parent directory — and the framework doesn't guarantee it exists.
ExecutionEnvironment.testOutputFilenameFor only builds a Path (no mkdirs). The only directory creation is in AutotestEngine.configureLogging, which covers the before/after stages, but the main stage builds its ExecutionEnvironment with a differently-derived display name (AutotestEngine.java:331 vs :174), so the directory can be missing when this act runs → NoSuchFileException → RuntimeException aborts the step. The web Screenshot never hits this because Playwright's setPath auto-creates parent directories. The unit test masks the issue by mocking the destination to an already-existing temp file.
Suggestion: Files.createDirectories(destination.getParent()) before the copy.
Also, minor: testOutputFilenameFor(...) already returns a java.nio.file.Path — the new File(String.valueOf(...)).toPath() round-trip can be dropped and the Path passed to Files.copy directly.
There was a problem hiding this comment.
Files.mkdirsだったかな?参考にするといいかも
There was a problem hiding this comment.
ディレクトリ作成にはFiles.mkdirsとFiles.createDirectoriesがあるようですが、どちらを採用するのがいいと思いますか?個人的には複数のexceptionを投げるcreateDirectoriesの方が良いように思えるのですが...
There was a problem hiding this comment.
上のように書いちゃいましたがFile.mkdirsはレガシーですね、Files.createDirectoriesの方がいいと思います。
There was a problem hiding this comment.
こちらのcommitでFiles.createDirectoriesを導入するように修正しました。
| /// | ||
| @Override | ||
| public AppiumDriver perform(AppiumDriver value, ExecutionEnvironment executionEnvironment) { | ||
| File screenshot = value.getScreenshotAs(OutputType.FILE); |
There was a problem hiding this comment.
[8] Per-screenshot temp-file leak and double write.
getScreenshotAs(OutputType.FILE) writes the image to a Selenium-created temp file that is deleted only on JVM exit, and nothing deletes it here after the copy — one leaked temp file per screenshot, at every beforeAll/beforeEach/afterEach/afterAll across a long session, plus a redundant second disk write.
Suggestion: Files.write(destination, value.getScreenshotAs(OutputType.BYTES)) — one write, no temp file.
| /// | ||
| /// A prefix to control a | ||
| /// | ||
| public static final String MASK_PREFIX = "MASK!"; |
There was a problem hiding this comment.
[5] MASK_PREFIX is redefined here, but the framework's masking is bound to the web constant.
InternalUtils.java:34 does import static jp.co.moneyforward.autotest.actions.web.SendKey.MASK_PREFIX; and uses it in mask() (InternalUtils.java:150). With two independent "MASK!" literals, a future change to either silently breaks the other: secrets in mobile flows would stop being masked by framework-level logging while web flows stay masked.
Suggestion: reference one shared constant (e.g. move MASK_PREFIX to a neutral home like InternalUtils and have both SendKey classes point at it, or have this class reference web.SendKey.MASK_PREFIX).
|
|
||
| import static com.github.valid8j.classic.Requires.requireNonNull; | ||
|
|
||
| public class SendKey implements Act<AppiumDriver, AppiumDriver> { |
There was a problem hiding this comment.
[6] Missing name() override — action trees lose the target locator.
The web SendKey overrides name() to print SendKey[locator][MASK!|keys], and the sibling ClickBase in this package also overrides name(). This class falls back to the default Act.name(), so a failing step renders as bare SendKey with no indication of which field was targeted (and the documented "MASK_PREFIX is printed in the log" behavior is non-functional — though, to be clear, nothing leaks).
Suggestion: port the web name() override, including its masking branch.
| return Printables.function("title", AppiumDriver::getTitle); | ||
| } | ||
|
|
||
| public static Function<AppiumDriver, WebElement> linkLocatorByText(String text, boolean lenient) { |
There was a problem hiding this comment.
[7] linkLocatorByText builds byte-identical XPath to locatorByText — nothing restricts it to link-like elements.
Only the Printables label differs (link:@[...] vs @[...]). The Javadoc promises "a link-like element", and the web counterpart genuinely restricts via getByRole(AriaRole.LINK, ...), but this XPath matches //*. A user relying on the doc to disambiguate a link from a plain label with the same text gets an arbitrary matching node.
Suggestion: either delegate to locatorByText and fix the docs, or actually filter to link-like widget classes. The duplicated XPath string should collapse into one place either way.
Follow-up: gaps vs. the
|
|
基本的にはこのPRの範囲外だと思いますが:
もっとも”Hello, world"レベルのモバイル用の実例はこのPRに含められると嬉しいだろうなと思います。(SUTをどうしよう?と言うのはありますが) |
…with hidden views
Summary
This Pull Request adds actions for mobile and introduces capabilities to support mobile testing
Changes:
AppiumDriverunderactions/mobileBuiltInActsTestfor the newly introduced actionsjava-client(Appium) to dependenciesVerification: