CI: Add blank app fixture runner and appium tests #880
Conversation
Summary of ChangesHello @descorp, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the project's testing capabilities by introducing an automated integration testing suite. It sets up a process to build and test the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
descorp
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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 introduces a valuable set of scripts for creating a test fixture app and running Appium tests, which is a great step towards automated integration testing. My review has identified a few critical and high-severity bugs in the shell scripts related to argument parsing and environment variable handling that would prevent them from executing correctly. I have also included several suggestions to improve the robustness of the tests and the efficiency of the scripts. Addressing these points will ensure the new testing workflow is reliable and maintainable.
descorp
force-pushed
the
ci-blank-tests
branch
from
26d4e52 to
c6f7eff
Compare
descorp
force-pushed
the
ci-blank-tests
branch
from
c6f7eff to
1534443
Compare
descorp
force-pushed
the
ci-blank-tests
branch
from
1534443 to
e449ffb
Compare
descorp
force-pushed
the
ci-blank-tests
branch
from
e449ffb to
f0a5a1e
Compare
descorp
force-pushed
the
ci-blank-tests
branch
from
f0a5a1e to
9d210f5
Compare
descorp
force-pushed
the
ci-blank-tests
branch
from
9d210f5 to
b680499
Compare
descorp
force-pushed
the
ci-blank-tests
branch
2 times, most recently
from
b7e3f3a to
6baa0f3
Compare
descorp
force-pushed
the
ci-blank-tests
branch
from
6baa0f3 to
0580c73
Compare
descorp
force-pushed
the
ci-blank-tests
branch
from
8f2f01a to
f50c8be
Compare
descorp
force-pushed
the
ci-blank-tests
branch
from
f50c8be to
de76df4
Compare
descorp
force-pushed
the
ci-blank-tests
branch
from
de76df4 to
779fc06
Compare
|
.github/workflows/blank_test.yml
Outdated
| run: | | ||
| appium --port 4723 --log-level error & | ||
| for i in {1..90}; do | ||
| if curl -sf http://127.0.0.1:4723/status >/dev/null; then |
There was a problem hiding this comment.
is this waiting for a server to start up and be ready?
There was a problem hiding this comment.
Correct. On Ubuntu it happens pretty fast, but on Mac it take some significant time.
atmamont
left a comment
atmamont
left a comment
There was a problem hiding this comment.
Amazing investment into reliability, Vlad.
I have some concerns regarding injecting secrets in the script runs using sed as it might be a sec vulnerability as they will be visible in logs.
I am not a fan of bash scripts as they are hard to maintain for anyone but author, let me take some time to think of what could be improved in terms of readability.
| adb reverse tcp:8081 tcp:8081 || true | ||
|
|
||
| echo "== Launch App" | ||
| adb shell am start -n "$APP_PACKAGE/$APP_ACTIVITY" >/dev/null 2>&1 || true |
There was a problem hiding this comment.
|| true means it will pass it the command fails, why wouldn't we fail early here?
scripts/fixture_ios.sh
Outdated
| export IOS_UDID="$UDID" | ||
| export IOS_DEVICE_NAME="$device_name" | ||
| export IOS_PLATFORM_VERSION="$os_version" | ||
| node ./check-app.js No newline at end of file |
There was a problem hiding this comment.
This is quite a complex setup and is not scalable for other platforms(at least flutter). Let's think of alternative solutions?
Using meta repository pipelines that run nightly or upon a new release could be more scalable, as it uses appium repo as a submodule and you can get rid of all wrapper and setup code that would be a hell to maintain.
scripts/fixture_android.sh
Outdated
| fi | ||
|
|
||
| echo "== Start Metro" | ||
| yarn start --port 8081 >/dev/null 2>&1 & |
There was a problem hiding this comment.
This could leave bundler running if script fails (I am not sure but it can easily burn github runner paid minutes). It's a general recommendation to catch such things, here is one suggestion from LLM:
cleanup() {
pkill -f "node.*metro" || true
}
trap cleanup EXIT
|
@Robert-SD we need your input here While this is definitely a nice improvement to react native, I feel it would be hard to scale to all platforms. Key issues:
Rather than maintaining separate Appium infrastructure per platform, I suggest we extend the existing adyen-checkout-e2e-testing to include React Native: This aligns with the meta repository vision where every effort scales to all mobile platforms automatically. Note: This is not a blocker for merging - just an invitation to think bigger picture about "SDK development" vs "platform-specific development". |
|
@descorp @atmamont I really like the addition and will put the PR into perspective. Here are my thoughts:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new CI workflow for running integration tests on a blank app using Appium. The changes include scripts for setting up the test fixture, running tests on both Android and iOS, and the Appium test script itself.
My review focuses on improving the robustness and maintainability of these new scripts. I've identified a couple of high-severity issues in the shell scripts related to waiting for the Metro server, which could lead to flaky CI runs. I've also found opportunities for refactoring in the Appium test script to reduce code duplication and a minor cleanup in the generated test app code. Overall, this is a great addition for improving test coverage.
scripts/fixture_android.sh
Outdated
| for i in {1..30}; do | ||
| nc -z 127.0.0.1 8081 && break | ||
| sleep 1 | ||
| done |
There was a problem hiding this comment.
This loop waits for Metro to start, but it doesn't handle the case where Metro fails to start within the 30-second timeout. The script will continue execution, likely causing subsequent steps to fail with less clear error messages. It's better to exit with an error if the loop completes without Metro starting.
Consider changing the loop to something like this:
for i in {1..30}; do
if nc -z 127.0.0.1 8081; then
break
fi
if [ "$i" -eq 30 ]; then
echo "Error: Metro failed to start after 30 seconds."
exit 1
fi
sleep 1
done
scripts/fixture_ios.sh
Outdated
| for i in {1..30}; do | ||
| nc -z 127.0.0.1 8081 && break | ||
| sleep 1 | ||
| done |
There was a problem hiding this comment.
This loop waits for Metro to start, but it doesn't handle the case where Metro fails to start within the 30-second timeout. The script will continue execution, likely causing subsequent steps to fail with less clear error messages. It's better to exit with an error if the loop completes without Metro starting.
Consider changing the loop to something like this:
for i in {1..30}; do
if nc -z 127.0.0.1 8081; then
break
fi
if [ "$i" -eq 30 ]; then
echo "Error: Metro failed to start after 30 seconds."
exit 1
fi
sleep 1
done| try { | ||
| const screenshotPath = './appium_failure.png'; | ||
| await driver.saveScreenshot(screenshotPath); | ||
| console.error(`==> [Test] Saved screenshot to ${screenshotPath}`); | ||
| } catch (e) { | ||
| console.error('==> [Test] Failed to capture screenshot'); | ||
| console.error(e); | ||
| } |
There was a problem hiding this comment.
The logic for saving a screenshot is duplicated in this file (here and in the final catch block on lines 145-152). To improve maintainability and reduce code duplication, you could extract this logic into a helper function. Also, the screenshot path './appium_failure.png' is hardcoded in two places and could be defined as a constant.
Here's a suggested refactoring:
const SCREENSHOT_PATH = './appium_failure.png';
async function saveScreenshot(driver) {
try {
await driver.saveScreenshot(SCREENSHOT_PATH);
console.error(`==> [Test] Saved screenshot to ${SCREENSHOT_PATH}`);
} catch (e) {
console.error('==> [Test] Failed to capture screenshot');
console.error(e);
}
}You can then replace the screenshot logic in both places with a single call: await saveScreenshot(driver);
scripts/fixture_android.sh
Outdated
|
|
||
| name=$1 | ||
| platform=$2 | ||
| api_level=$3 |
There was a problem hiding this comment.
The api_level variable is assigned from the third script argument but it's never used. It should either be used or removed to avoid confusion.
scripts/fixture_setup.sh
Outdated
| <View> | ||
| <StatusBar barStyle={isDarkMode ? 'light-content' : 'dark-content'} /> | ||
| <ScrollView contentInsetAdjustmentBehavior="automatic"> | ||
| <View style={{}}> |
There was a problem hiding this comment.
The View component has an empty style object (style={{}}). This is unnecessary and can be removed.
| <View style={{}}> | |
| <View> |
descorp
force-pushed
the
ci-blank-tests
branch
from
4922b1b to
576ea33
Compare
3 participants
Changes