Use stagemole with e2e tests #7343
Conversation
There was a problem hiding this comment.
Copilot reviewed 1 out of 4 changed files in this pull request and generated no comments.
Files not reviewed (3)
- android/scripts/pull-test-output.sh: Language not supported
- android/scripts/run-instrumented-tests.sh: Language not supported
- android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/SimpleMullvadHttpClient.kt: Language not supported
niklasberglund
force-pushed
the
use-stagemole-in-self-hosted-e2e-tests-droid-1561
branch
3 times, most recently
from
b63b68a to
b067b08
Compare
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
There was a problem hiding this comment.
Reviewed 3 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @niklasberglund)
android/scripts/run-instrumented-tests.sh line 112 at r1 (raw file):
echo "Error: The 'e2e' test type with billing flavor 'play' require infra flavor 'stagemole'." exit 1 elif [[ $BILLING_FLAVOR == "oss" && $INFRA_FLAVOR != "prod" ]]; then
Should this block really be removed? Didn't we say to keep using the play build? 🤔
.github/workflows/android-app.yml line 51 at r1 (raw file):
e2e_tests_infra_flavor: description: > Infra environment to run e2e tests on(prod/stagemole).
nit: add space on (prod/stagemole)
Code quote:
on(prod/stagemole)..github/workflows/android-app.yml line 351 at r1 (raw file):
- name: Build stagemole app uses: burrunan/gradle-cache-action@v1 if: github.event.inputs.e2e_test_repeat != '0'
Can we check e2e_tests_infra_flavor instead?
Code quote:
github.event.inputs.e2e_test_repeat != '0'.github/workflows/android-app.yml line 494 at r1 (raw file):
- name: Upload instrumentation report (${{ matrix.test-type }}) uses: actions/upload-artifact@v4 if: always() && matrix.test-repeat != 0 && github.event.inputs.e2e_tests_infra_flavor == 'stagemole'
Do we need this check? These tests doesn't include anything sensitive since it's only used for isolated tests atm (unless we decide to merge instrumented-tests & instrumented-e2e-tests
Code quote:
&& github.event.inputs.e2e_tests_infra_flavor == 'stagemole'.github/workflows/android-app.yml line 501 at r1 (raw file):
retention-days: 7 instrumented-e2e-tests:
Could it make sense to merge this one with instrumented-tests now that the difference between the two is less than before? I believe the main thing to consider is that we would only want to set some of the env variables for the e2e tests (such as PARTNER_AUTH).
Code quote:
instrumented-e2e-testsandroid/scripts/pull-test-output.sh line 35 at r1 (raw file):
LOCAL_LOGCAT_FILE_PATH="$REPORT_DIR/logcat.txt" LOCAL_SCREENSHOT_PATH="$REPORT_DIR/screenshots" LOCAL_TEST_ARRACHMENTS_PATH="$REPORT_DIR/test-attachments"
Would be really nice if we could just have a single on-device path for all this content such as /sdcard/test-report that we just pull recursively into $REPORT_DIR instead of a bunch of paths that we now have to keep track of in two different scripts. But probably a bit out-of-scope for this PR...
Code quote:
DEVICE_SCREENSHOT_PATH="/sdcard/Pictures/mullvad-$TEST_TYPE"
DEVICE_TEST_ATTACHMENTS_PATH="/sdcard/Download/test-attachments"
LOCAL_LOGCAT_FILE_PATH="$REPORT_DIR/logcat.txt"
LOCAL_SCREENSHOT_PATH="$REPORT_DIR/screenshots"
LOCAL_TEST_ARRACHMENTS_PATH="$REPORT_DIR/test-attachments"android/scripts/pull-test-output.sh line 38 at r1 (raw file):
echo "Collecting report and produced test attachments..." adb logcat -d > "$LOCAL_LOGCAT_FILE_PATH"
The other script that runs the tests should probably be responsible for dumping the logcat content into a file on the device. This script can then be responsible for downloading that file.
Code quote:
adb logcat -d > "$LOCAL_LOGCAT_FILE_PATH"
niklasberglund
force-pushed
the
use-stagemole-in-self-hosted-e2e-tests-droid-1561
branch
from
40e8539 to
b5883b8
Compare
There was a problem hiding this comment.
Reviewable status: 1 of 8 files reviewed, 7 unresolved discussions (waiting on @albin-mullvad and @Pururun)
.github/workflows/android-app.yml line 51 at r1 (raw file):
Previously, albin-mullvad wrote…
nit: add space
on (prod/stagemole)
Added space. Also added to mockapi_test_repeat and e2e_test_repeat descriptions
.github/workflows/android-app.yml line 351 at r1 (raw file):
Previously, albin-mullvad wrote…
Can we check
e2e_tests_infra_flavorinstead?
Now checking e2e_test_repeat, run_firebase_tests and e2e_tests_infra_flavor
.github/workflows/android-app.yml line 494 at r1 (raw file):
Previously, albin-mullvad wrote…
Do we need this check? These tests doesn't include anything sensitive since it's only used for isolated tests atm (unless we decide to merge
instrumented-tests&instrumented-e2e-tests
Removed the stagemole check
.github/workflows/android-app.yml line 501 at r1 (raw file):
Previously, albin-mullvad wrote…
Could it make sense to merge this one with
instrumented-testsnow that the difference between the two is less than before? I believe the main thing to consider is that we would only want to set some of theenvvariables for the e2e tests (such asPARTNER_AUTH).
Created triage issue for this. We had an offline talk about keeping it outside of this PR.
android/scripts/pull-test-output.sh line 35 at r1 (raw file):
Previously, albin-mullvad wrote…
Would be really nice if we could just have a single on-device path for all this content such as
/sdcard/test-reportthat we just pull recursively into$REPORT_DIRinstead of a bunch of paths that we now have to keep track of in two different scripts. But probably a bit out-of-scope for this PR...
Now all files are under /sdcard/Downloads/test-outputs
android/scripts/pull-test-output.sh line 38 at r1 (raw file):
Previously, albin-mullvad wrote…
The other script that runs the tests should probably be responsible for dumping the logcat content into a file on the device. This script can then be responsible for downloading that file.
Dumping the content in the other file now
android/scripts/run-instrumented-tests.sh line 112 at r1 (raw file):
Previously, albin-mullvad wrote…
Should this block really be removed? Didn't we say to keep using the play build? 🤔
Brought this back
There was a problem hiding this comment.
Reviewable status: 1 of 8 files reviewed, 8 unresolved discussions (waiting on @albin-mullvad and @Pururun)
.github/workflows/android-app.yml line 560 at r2 (raw file):
INFRA_FLAVOR: ${{ github.event.inputs.e2e_tests_infra_flavor }} PARTNER_AUTH: ${{ github.event.inputs.e2e_tests_infra_flavor == 'stagemole' && secrets.STAGEMOLE_PARTNER_AUTH || '' }} VALID_TEST_ACCOUNT_NUMBER: ${{ github.event.inputs.e2e_tests_infra_flavor == 'prod' && secrets.ANDROID_PROD_TEST_ACCOUNT || '' }}
The two lines above are too long and should be split into multiple lines, working on splitting them without breaking the code
There was a problem hiding this comment.
Reviewable status: 1 of 8 files reviewed, 9 unresolved discussions (waiting on @albin-mullvad and @Pururun)
android/test/common/src/main/kotlin/net/mullvad/mullvadvpn/test/common/rule/CaptureScreenshotOnFailedTestRule.kt line 20 at r2 (raw file):
import java.time.OffsetDateTime import java.time.temporal.ChronoUnit import net.mullvad.mullvadvpn.test.common.misc.Attachment
Only using Attachment for the outputs path right now. Should probably also use it for storing screenshots though?
niklasberglund
force-pushed
the
use-stagemole-in-self-hosted-e2e-tests-droid-1561
branch
from
e2adfa4 to
ae42fb2
Compare
Using stagemole for e2e tests by default. Can optionally specify to use prod. Artefacts will be uploaded if testing with stagemole.
Also added very basic error handling for 429 rate limited error responses(very basic since we probably will migrate to ktor) and moved pulling of outputs from
run-instrumented-tests.shto the workflow, so that artefacts are uploaded even if the run is cancelled or tests step time out. Fixes DROID-1635This change is