Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Update hashing and iteration logic of page object items #2067

Merged
merged 11 commits into from
Nov 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions .github/workflows/gradle.yml
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,12 @@ jobs:
cache: 'gradle'

- name: Build with Gradle
run: ./gradlew clean build
run: |
latest_snapshot=$(curl -sf https://oss.sonatype.org/content/repositories/snapshots/org/seleniumhq/selenium/selenium-api/ | \
python -c "import sys,re; print(re.findall(r'\d+\.\d+\.\d+-SNAPSHOT', sys.stdin.read())[-1])")
echo ">>> $latest_snapshot"
echo "latest_snapshot=$latest_snapshot" >> "$GITHUB_ENV"
./gradlew clean build -PisCI -Pselenium.version=$latest_snapshot
valfirst marked this conversation as resolved.
Show resolved Hide resolved

- name: Install Node.js
if: matrix.e2e-tests == 'android' || matrix.e2e-tests == 'ios'
Expand All @@ -76,7 +81,7 @@ jobs:
if: matrix.e2e-tests == 'android'
uses: reactivecircus/android-emulator-runner@v2
with:
script: ./gradlew uiAutomationTest
script: ./gradlew uiAutomationTest -PisCI -Pselenium.version=$latest_snapshot
api-level: ${{ env.ANDROID_SDK_VERSION }}
avd-name: ${{ env.ANDROID_EMU_NAME }}
sdcard-path-or-size: 1500M
Expand All @@ -103,4 +108,4 @@ jobs:
xcrun simctl bootstatus $target_sim_id -b
- name: Run iOS E2E tests
if: matrix.e2e-tests == 'ios'
run: ./gradlew xcuiTest
run: ./gradlew xcuiTest -PisCI -Pselenium.version=$latest_snapshot
50 changes: 30 additions & 20 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,15 @@ plugins {

repositories {
mavenCentral()

if (project.hasProperty("isCI")) {
maven {
url uri('https://oss.sonatype.org/content/repositories/snapshots/')
mavenContent {
snapshotsOnly()
}
}
}
}

java {
Expand All @@ -32,22 +41,28 @@ dependencies {
compileOnly 'org.projectlombok:lombok:1.18.30'
annotationProcessor 'org.projectlombok:lombok:1.18.30'

api ('org.seleniumhq.selenium:selenium-api') {
version {
strictly "[${seleniumVersion}, 5.0)"
prefer "${seleniumVersion}"
if (project.hasProperty("isCI")) {
api "org.seleniumhq.selenium:selenium-api:${seleniumVersion}"
api "org.seleniumhq.selenium:selenium-remote-driver:${seleniumVersion}"
api "org.seleniumhq.selenium:selenium-support:${seleniumVersion}"
} else {
api('org.seleniumhq.selenium:selenium-api') {
version {
strictly "[${seleniumVersion}, 5.0)"
prefer "${seleniumVersion}"
}
}
}
api ('org.seleniumhq.selenium:selenium-remote-driver') {
version {
strictly "[${seleniumVersion}, 5.0)"
prefer "${seleniumVersion}"
api('org.seleniumhq.selenium:selenium-remote-driver') {
version {
strictly "[${seleniumVersion}, 5.0)"
prefer "${seleniumVersion}"
}
}
}
api ('org.seleniumhq.selenium:selenium-support') {
version {
strictly "[${seleniumVersion}, 5.0)"
prefer "${seleniumVersion}"
api('org.seleniumhq.selenium:selenium-support') {
version {
strictly "[${seleniumVersion}, 5.0)"
prefer "${seleniumVersion}"
}
}
}
implementation 'com.google.code.gson:gson:2.10.1'
Expand All @@ -59,11 +74,7 @@ dependencies {
testImplementation (group: 'io.github.bonigarcia', name: 'webdrivermanager', version: '5.6.1') {
exclude group: 'org.seleniumhq.selenium'
}
testImplementation platform(group: 'org.seleniumhq.selenium', name: 'selenium-bom', version: '4.15.0')
testImplementation 'org.seleniumhq.selenium:selenium-api'
testImplementation 'org.seleniumhq.selenium:selenium-remote-driver'
testImplementation 'org.seleniumhq.selenium:selenium-support'
testImplementation 'org.seleniumhq.selenium:selenium-chrome-driver'
testImplementation "org.seleniumhq.selenium:selenium-chrome-driver:${seleniumVersion}"
testRuntimeOnly "org.slf4j:slf4j-simple:${slf4jVersion}"
}

Expand Down Expand Up @@ -228,7 +239,6 @@ tasks.register('uiAutomationTest', Test) {
includeTestsMatching 'io.appium.java_client.android.OpenNotificationsTest'
includeTestsMatching '*.AndroidAppStringsTest'
includeTestsMatching '*.pagefactory_tests.widget.tests.android.*'
includeTestsMatching '*.pagefactory_tests.widget.tests.AndroidPageObjectTest'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it removed intentionally?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. The current pattern does not match anything. I've fixed it, but these tests still fail because of the CI slowness (I've verified them locally though). Maybe someone would find some time to make them work in the slow CI env 🤷

includeTestsMatching 'io.appium.java_client.service.local.StartingAppLocallyAndroidTest'
includeTestsMatching 'io.appium.java_client.service.local.ServerBuilderTest'
includeTestsMatching 'io.appium.java_client.service.local.ThreadSafetyTest'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,17 +61,15 @@ public ByChained(By[] bys) {
@Override
public WebElement findElement(SearchContext context) {
Function<SearchContext, WebElement> searchingFunction = null;

for (By by: bys) {
searchingFunction = Optional.ofNullable(searchingFunction != null
? searchingFunction.andThen(getSearchingFunction(by)) : null).orElse(getSearchingFunction(by));
searchingFunction = Optional.ofNullable(searchingFunction)
.map(sf -> sf.andThen(getSearchingFunction(by)))
.orElseGet(() -> getSearchingFunction(by));
}

FluentWait<SearchContext> waiting = new FluentWait<>(context);
requireNonNull(searchingFunction);

try {
requireNonNull(searchingFunction);
return waiting.until(searchingFunction);
return new FluentWait<>(context).until(searchingFunction);
} catch (TimeoutException e) {
throw new NoSuchElementException("Cannot locate an element using " + this);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,11 @@ protected abstract Object getObject(

@Override
public Object call(Object obj, Method method, Object[] args, Callable<?> original) throws Throwable {
if (locator == null || Object.class.equals(method.getDeclaringClass())) {
if (locator == null || Object.class == method.getDeclaringClass()) {
return original.call();
}

List<WebElement> realElements = new ArrayList<>(locator.findElements());
final var realElements = new ArrayList<>(locator.findElements());
return getObject(realElements, method, args);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,13 @@
import org.openqa.selenium.WebDriver;
import org.openqa.selenium.WebElement;
import org.openqa.selenium.WrapsDriver;
import org.openqa.selenium.remote.RemoteWebElement;
import org.openqa.selenium.support.pagefactory.ElementLocator;

import javax.annotation.Nullable;
import java.lang.ref.WeakReference;
import java.lang.reflect.Method;
import java.util.Objects;
import java.util.concurrent.Callable;

public abstract class InterceptorOfASingleElement implements MethodCallListener {
Expand All @@ -42,6 +44,15 @@ public InterceptorOfASingleElement(

protected abstract Object getObject(WebElement element, Method method, Object[] args) throws Throwable;

private static boolean areElementsEqual(Object we1, Object we2) {
if (!(we1 instanceof RemoteWebElement) || !(we2 instanceof RemoteWebElement)) {
return false;
}

return we1 == we2
|| (Objects.equals(((RemoteWebElement) we1).getId(), ((RemoteWebElement) we2).getId()));
}

@Override
public Object call(Object obj, Method method, Object[] args, Callable<?> original) throws Throwable {
if (locator == null) {
Expand All @@ -52,7 +63,7 @@ public Object call(Object obj, Method method, Object[] args, Callable<?> origina
return locator.toString();
}

if (Object.class.equals(method.getDeclaringClass())) {
if (Object.class == method.getDeclaringClass()) {
return original.call();
}

Expand All @@ -62,6 +73,9 @@ public Object call(Object obj, Method method, Object[] args, Callable<?> origina
}

WebElement realElement = locator.findElement();
if ("equals".equals(method.getName()) && args.length == 1) {
return areElementsEqual(realElement, args[0]);
}
return getObject(realElement, method, args);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,8 @@
* proxy object here.
*/
public final class ProxyFactory {
private static final Set<String> NON_PROXYABLE_METHODS = setWith(
setWithout(OBJECT_METHOD_NAMES, "toString"),
"iterator"
private static final Set<String> NON_PROXYABLE_METHODS = setWithout(
OBJECT_METHOD_NAMES, "toString", "equals", "hashCode"
);

@SafeVarargs
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import io.appium.java_client.pagefactory.AppiumFieldDecorator;
import io.appium.java_client.pagefactory.HowToUseLocators;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.openqa.selenium.NoSuchElementException;
import org.openqa.selenium.WebElement;
Expand All @@ -34,16 +35,19 @@
import org.openqa.selenium.support.PageFactory;

import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;

import static io.appium.java_client.pagefactory.LocatorGroupStrategy.ALL_POSSIBLE;
import static java.time.Duration.ofSeconds;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;

@SuppressWarnings({"unused", "MismatchedQueryAndUpdateOfCollection"})
public class AndroidPageObjectTest extends BaseAndroidTest {

private boolean populated = false;
Expand Down Expand Up @@ -149,6 +153,10 @@ public class AndroidPageObjectTest extends BaseAndroidTest {
@FindBy(id = "fakeId")
private List<WebElement> fakeElements;

@FindBy(className = "android.widget.TextView")
@CacheLookup
private List<WebElement> cachedViews;

@CacheLookup
@FindBy(className = "android.widget.TextView")
private WebElement cached;
Expand Down Expand Up @@ -343,8 +351,22 @@ public class AndroidPageObjectTest extends BaseAndroidTest {
assertNotEquals(ArrayList.class, fakeElements.getClass());
}

@Test public void checkCached() {
@Test public void checkCachedElements() {
assertEquals(((RemoteWebElement) cached).getId(), ((RemoteWebElement) cached).getId());
assertEquals(cached.hashCode(), cached.hashCode());
//noinspection SimplifiableAssertion,EqualsWithItself
assertTrue(cached.equals(cached));
}

@Test public void checkCachedLists() {
assertEquals(cachedViews.hashCode(), cachedViews.hashCode());
//noinspection SimplifiableAssertion,EqualsWithItself
assertTrue(cachedViews.equals(cachedViews));
}

@Test public void checkListHashing() {
assertFalse(cachedViews.isEmpty());
assertEquals(cachedViews.size(), new HashSet<>(cachedViews).size());
}

@Test
Expand All @@ -364,6 +386,7 @@ public void checkThatElementSearchingThrowsExpectedExceptionIfChainedLocatorIsIn
assertNotEquals(0, androidElementsViewFoundByMixedSearching.size());
}

@Disabled("FIXME")
@Test public void checkMixedElementSearching2() {
assertNotNull(androidElementViewFoundByMixedSearching2.getAttribute("text"));
}
Expand Down