-
Notifications
You must be signed in to change notification settings - Fork 352
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 e2e and mockapi tests failing #6610
Fix e2e and mockapi tests failing #6610
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @niklasberglund)
android/test/mockapi/build.gradle.kts
line 93 at r1 (raw file):
implementation(Dependencies.junit5AndroidTestCompose) implementation(libs.compose.material3)
I suggest trying to include implementation(libs.compose.ui)
as well to see if that helps gradle use newer versions of various dependencies
android/test/mockapi/build.gradle.kts
line 94 at r1 (raw file):
implementation(Dependencies.junit5AndroidTestCompose) implementation(libs.compose.material3) }
We should make similar adjustments to android/test/e2e/build.gradle.kts
and also make sure we're able to run those tests.
3f04351
to
629b7eb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 8 files reviewed, 1 unresolved discussion (waiting on @albin-mullvad)
android/test/mockapi/build.gradle.kts
line 93 at r1 (raw file):
Previously, albin-mullvad wrote…
I suggest trying to include
implementation(libs.compose.ui)
as well to see if that helps gradle use newer versions of various dependencies
We tried adding it and it did help 👍
android/test/mockapi/build.gradle.kts
line 94 at r1 (raw file):
Previously, albin-mullvad wrote…
We should make similar adjustments to
android/test/e2e/build.gradle.kts
and also make sure we're able to run those tests.
Added and made sure e2e tests also run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 8 of 8 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @albin-mullvad)
5855ca2
to
780149d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 7 of 7 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @niklasberglund)
.github/workflows/android-app.yml
line 472 at r3 (raw file):
instrumented-e2e-tests: name: Run instrumented e2e tests runs-on: [self-hosted, android-device, android-runner-v1] # Temporary until we can add secrets
I suggest clarifying that this refers specifically to android-runner-v1
. Would also be nice to move the comment to the line above since the suggestion would result in a quite long line
Code quote:
# Temporary until we can add secrets
.github/workflows/android-app.yml
line 504 at r3 (raw file):
run: | if [ ${{github.event_name}} == 'schedule' ]; then echo "ignore_highly_rate_limited_tests=false" >> $GITHUB_ENV
I believe it would be a good idea to invert this variable to avoid a double-false which can easily be misunderstood. So e.g. use the following here: enable_highly_rate_limited_tests=true
Code quote:
ignore_highly_rate_limited_tests=false
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/annotations/HighlyRateLimited.kt
line 23 at r3 (raw file):
Can this be shortened a bit? The following should be sufficient and also matches the enabled
case:
Skipping test test highly affected by rate limiting.
Code quote:
Skipping test because this run is configured to skip tests that are highly affected by rate limiting
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/annotations/HighlyRateLimited.kt
line 25 at r3 (raw file):
return ConditionEvaluationResult.disabled("Skipping test because this run is configured to skip tests that are highly affected by rate limiting") } else { return ConditionEvaluationResult.enabled("Running test highly affected by rate limiting")
nit: please add a dot
Code quote:
limiting")
780149d
to
e96fc2a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 5 files at r4, all commit messages.
Reviewable status: 14 of 15 files reviewed, 4 unresolved discussions (waiting on @niklasberglund)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: 13 of 15 files reviewed, 5 unresolved discussions (waiting on @niklasberglund)
-- commits
line 22 at r5:
Too long and a bit difficult to parse, please try to clarify. E.g. is it running or ignoring the mentioned tests?
Code quote:
Both firebase and self hosted e2e runs ignoring rate limited tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 8 files at r6, all commit messages.
Reviewable status: 11 of 15 files reviewed, 3 unresolved discussions (waiting on @niklasberglund and @Pururun)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 8 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @niklasberglund)
968ea73
to
0911a8b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 12 of 15 files reviewed, 3 unresolved discussions (waiting on @albin-mullvad and @Pururun)
Previously, albin-mullvad wrote…
Too long and a bit difficult to parse, please try to clarify. E.g. is it running or ignoring the mentioned tests?
Squashed
.github/workflows/android-app.yml
line 472 at r3 (raw file):
Previously, albin-mullvad wrote…
I suggest clarifying that this refers specifically to
android-runner-v1
. Would also be nice to move the comment to the line above since the suggestion would result in a quite long line
See updated comment
.github/workflows/android-app.yml
line 504 at r3 (raw file):
Previously, albin-mullvad wrote…
I believe it would be a good idea to invert this variable to avoid a double-false which can easily be misunderstood. So e.g. use the following here:
enable_highly_rate_limited_tests=true
Much better 👍. Changed to enable_highly_rate_limited_tests
and ENABLE_HIGHLY_RATE_LIMITED_TESTS
.
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/annotations/HighlyRateLimited.kt
line 23 at r3 (raw file):
Previously, albin-mullvad wrote…
Can this be shortened a bit? The following should be sufficient and also matches the
enabled
case:Skipping test test highly affected by rate limiting.
Shortened
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/annotations/HighlyRateLimited.kt
line 25 at r3 (raw file):
Previously, albin-mullvad wrote…
nit: please add a dot
Added dot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 8 files at r6, 2 of 3 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @niklasberglund)
android/scripts/run-instrumented-tests.sh
line 134 at r9 (raw file):
exit 1 fi if [[ "${ENABLE_HIGHLY_RATE_LIMITED_TESTS}" == "false" ]]; then
Should we not set the test argument for both true
and false
? 🤔
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/ConnectionTest.kt
line 38 at r9 (raw file):
@Test @Disabled
Just to double check since I don't remember. Did we decide to disable this test for now?
Code quote:
@Disabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 8 files at r2, 1 of 8 files at r6, 2 of 2 files at r7.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @niklasberglund)
dad6611
to
82f1ef4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 12 of 15 files reviewed, 2 unresolved discussions (waiting on @albin-mullvad and @Pururun)
android/scripts/run-instrumented-tests.sh
line 134 at r9 (raw file):
Previously, albin-mullvad wrote…
Should we not set the test argument for both
true
andfalse
? 🤔
True, now setting for both and also made enable_highly_rate_limited_tests
optional with default value false
.
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/ConnectionTest.kt
line 38 at r9 (raw file):
Previously, albin-mullvad wrote…
Just to double check since I don't remember. Did we decide to disable this test for now?
Yes we said it can be re-introduced later. Created an issue for it https://linear.app/mullvad/issue/DROID-1262/fix-failing-test-testconnectandverifywithconnectioncheck
The useful description to go with @Disabled
has been removed though, I have now added it again.
82f1ef4
to
f60303e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
android/scripts/run-instrumented-tests.sh
line 134 at r9 (raw file):
Previously, niklasberglund (Niklas Berglund) wrote…
True, now setting for both and also made
enable_highly_rate_limited_tests
optional with default valuefalse
.
Nice! 👍
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/ConnectionTest.kt
line 38 at r9 (raw file):
Previously, niklasberglund (Niklas Berglund) wrote…
Yes we said it can be re-introduced later. Created an issue for it https://linear.app/mullvad/issue/DROID-1262/fix-failing-test-testconnectandverifywithconnectioncheck
The useful description to go with
@Disabled
has been removed though, I have now added it again.
Sounds good! 👍
f60303e
to
a141f19
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r12, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
a141f19
to
bcb4807
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r13, all commit messages.
Reviewable status: 15 of 16 files reviewed, all discussions resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r13, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
This PR contains fixes for making e2e and mockapi tests run.
enable_highly_rate_limited_tests
for specifying whether these tests should run or not.testCreateAccountAndLogout
.testLoginWithInvalidCredentials
to see if it is stable now.The changes in this PR can be tested by running the Android - Build and test workflow with e2e tests and firebase tests enabled.
This change is