Skip to content

Conversation

@ifernandezdiaz
Copy link
Contributor

🎟️ Tracking

📔 Objective

This PR streamlines and enhances our CI/CD pipeline by allowing the execution of sanity checks on real devices in our main build workflow.
It includes all necessary setup, secret retrieval, and artifact management to support E2E testing against real devices using SauceLabs.

📸 Screenshots

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@ifernandezdiaz ifernandezdiaz force-pushed the QA-1126b/adding-native-sanity-test branch from c6058a3 to 62b9088 Compare July 15, 2025 15:47
@codecov
Copy link

codecov bot commented Jul 18, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.40%. Comparing base (22114d5) to head (0aafc52).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5526   +/-   ##
=======================================
  Coverage   84.40%   84.40%           
=======================================
  Files         719      719           
  Lines       54764    54765    +1     
  Branches     7528     7528           
=======================================
+ Hits        46226    46227    +1     
  Misses       5897     5897           
  Partials     2641     2641           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@SaintPatrck SaintPatrck left a comment

Choose a reason for hiding this comment

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

Can the androidTest files be put in com.x8bit.bitwarden.* packages? I.e., com.x8bit.bitwarden.data.TestData.kt and com.x8bit.bitwarden.e2e.*.

Comment on lines +253 to +256
implementation(libs.androidx.uiautomator)
implementation(libs.androidx.espresso.core)
implementation(libs.androidx.junit.ktx)
implementation(libs.androidx.ui.test.junit4.android)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove these as implementation dependencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason, the build fails if I remove those dependencies 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think he want's them to be an androidTestImplementation to avoid having these test dependencies in the non-test builds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, but those deps are already present as androidTestImplementation in lines 302-306.
Apart from that, we need those deps in non-test builds since we aim to test the release build on real devices

Copy link
Collaborator

Choose a reason for hiding this comment

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

🤔 We do not want to ship with unnecessary dependencies like this. Is there a specific error you are seeing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't find a way to build the required testApp without those deps 🫤

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we need a new variant for this.

@vvolkgang vvolkgang force-pushed the QA-1126b/adding-native-sanity-test branch from 1f61ab4 to a4b464c Compare July 31, 2025 16:31
@vvolkgang vvolkgang force-pushed the QA-1126b/adding-native-sanity-test branch from a4b464c to f803752 Compare July 31, 2025 16:32
@ifernandezdiaz ifernandezdiaz force-pushed the QA-1126b/adding-native-sanity-test branch from 7d32d32 to bb8dda4 Compare August 4, 2025 17:48
# Keep Kotlin standard library classes
-keep class kotlin.** { *; }
-keep class kotlinx.** { *; }
-keep class kotlin.io.** { *; }
Copy link
Collaborator

@david-livefront david-livefront Aug 4, 2025

Choose a reason for hiding this comment

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

Why do we need this?

Are we obfuscating test code?

@ifernandezdiaz ifernandezdiaz force-pushed the QA-1126b/adding-native-sanity-test branch from eabd248 to 2cb6a6a Compare August 8, 2025 15:52
@ifernandezdiaz ifernandezdiaz force-pushed the QA-1126b/adding-native-sanity-test branch from 2cb6a6a to da9b60f Compare August 8, 2025 16:57
@vvolkgang vvolkgang marked this pull request as draft September 4, 2025 16:48
Comment on lines +288 to +304
# TODO: test if bundle exec fastlane assembleTestApk works and replace this step
# - name: Sign and rename test APK
# if: ${{ (matrix.variant == 'prod') && (matrix.artifact == 'apk') }}
# env:
# _TEST_APK_PATH: app/build/outputs/apk/androidTest/standard/release/com.x8bit.bitwarden-standard-release-androidTest.apk
# _TEST_APK_SIGNED_PATH: app/build/outputs/apk/androidTest/standard/release/com.x8bit.bitwarden-test.apk
# _PLAY_KEYSTORE_PASSWORD: ${{ steps.get-kv-secrets.outputs.PLAY-KEYSTORE-PASSWORD }}
# _PLAY_KEYSTORE_ALIAS: ${{ steps.get-kv-secrets.outputs.PLAY-KEYSTORE-ALIAS }}
# run: |
# $ANDROID_SDK_ROOT/build-tools/34.0.0/apksigner sign \
# --ks keystores/app_play-keystore.jks \
# --ks-key-alias bitwarden \
# --ks-pass pass:$_PLAY_KEYSTORE_PASSWORD \
# --key-pass pass:$_PLAY_KEYSTORE_PASSWORD \
# $_TEST_APK_PATH
# mv $_TEST_APK_PATH $_TEST_APK_SIGNED_PATH

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this TODO and commented code still needed? If so, can we associate it to a ticket so it isn't forgotten?

@ifernandezdiaz ifernandezdiaz marked this pull request as ready for review October 7, 2025 19:07
test-device:
name: Test device
needs: publish_playstore
uses: bitwarden/android/.github/workflows/test-device.yml@QA-1126b/adding-native-sanity-test #TODO replace branch with main before merging
Copy link
Contributor

Choose a reason for hiding this comment

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

This TODO needs to be addressed.

permissions:
contents: read
actions: read
checks: write
Copy link
Contributor

Choose a reason for hiding this comment

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

checks: write is only needed to create or update status checks. Neither are happening directly in the job, that I can see. I don't think this permission is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants