tests: enable automatic tests on Android. #3588
Conversation
bznein
force-pushed
the
testAndroid
branch
4 times, most recently
from
a187051 to
0201e65
Compare
bznein
changed the title
frontends/tests/e2e/base.test.js
Outdated
| if (driver) await driver.deleteSession(); | ||
| }); | ||
|
|
||
| it("App main page loads", async () => { |
There was a problem hiding this comment.
bznein
force-pushed
the
testAndroid
branch
6 times, most recently
from
126bca2 to
d582b04
Compare
|
It took me a while to realize that this is in frontends/tests and why and when we use frontends/web/tests Could you add a README.md to frontends/tests with some info what this is about so new devs can understand more easily? |
|
I added some docs; I also plan to spend today and tomorrow improving documentation around tests in general, so I will add a note to |
There was a problem hiding this comment.
untested LGTM
deferring to @benma as I am not soo familiar with CI/bash-script stuff.
.github/workflows/ci.yml
Outdated
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| api-level: [30, 31, 33, 34, 35] |
There was a problem hiding this comment.
I was thinking of only targeting API that were introduced with new Android versions, so I skipped 32 (which is part of Android 12 like level 31) but I changed it now
frontends/tests/e2e/base.test.js
Outdated
| 'appium:deviceName': 'Android Emulator', | ||
| 'appium:automationName': 'UiAutomator2', | ||
| 'appium:app': './apk/app-debug.apk', | ||
| 'appium:noReset': true, |
There was a problem hiding this comment.
What does this mean? Isn't it better to have a fresh state for each test? Maybe add some comments about what assumptions the tests should make in the README
There was a problem hiding this comment.
YEah you are probably right, I made many trial and error and I could have some leftover code that can be cleaned. I'll try without this flag and update the README if needed
|
|
||
| # --- Run tests --- | ||
| npm run test | ||
| TEST_STATUS=$? |
There was a problem hiding this comment.
If npm run test fails, doesn't the script abort due to set -e before we even get here?
If you did this so appium is killed on failure too, you could use trap instead:
set -e
npx appium ... &
APPIUM_PID=$!
trap 'kill -9 $APPIUM_PID' EXIT
...There was a problem hiding this comment.
Is this true?
If npm run test fails, doesn't the script abort due to set -e before we even get here?
There was a problem hiding this comment.
Sorry that's weird, I could swear I pushed a change to this line that addresses it, and you are right, no clue what I have done instead... I will push a change asap
.github/workflows/ci.yml
Outdated
| - name: Install Appium & drivers | ||
| run: | | ||
| npm install -g appium |
There was a problem hiding this comment.
In run.sh you use npx, so it's a different appium installation (locally managed by npx) that ends up being used. Unless I misunderstand, either this global installation is not needed, or the use of npx is not needed.
Also, should/can the version of appium be fixed, or is it preferable if it just always uses the latest?
There was a problem hiding this comment.
YEah I think both points make sense, I changed from npx to just appium and pinned to the current latest version
bznein
force-pushed
the
testAndroid
branch
3 times, most recently
from
61cee9d to
bb1e156
Compare
3 participants
Before asking for reviews, here is a check list of the most common things you might need to consider: