Skip to content

Conversation

@piotr-iohk
Copy link
Collaborator

@piotr-iohk piotr-iohk commented Nov 24, 2025

Description

Minor adjustments related to:

⚠️ merge together with iOS counterpart.

Preview

QA Notes

@claude
Copy link

claude bot commented Nov 24, 2025

PR Review: PinPrompt test id adjustment

Summary

This PR updates test IDs in the PinPrompt screen to align with cross-platform E2E testing conventions, removing hyphens from test identifiers.

✅ Positive Aspects

  • Consistency: Aligns Android test IDs with iOS implementation (Test/ios tags bitkit-ios#240) and E2E tests (Test ios tags bitkit-e2e-tests#53)
  • Naming Convention: The new naming (SecureWalletContinue) follows camelCase convention which is more consistent with other test IDs in the codebase
  • Documentation: The docs/e2e-test-ids.md file is properly updated to reflect the change

🐛 Critical Issue Found

Duplicate Test IDs: Both buttons in PinPromptScreen.kt have the same test ID:

  • Line 96: "Later" button → testTag("SecureWalletContinue")
  • Line 107: "Continue" button → testTag("SecureWalletContinue")

This is a bug that existed before this PR (both buttons previously had "SecureWallet-button-continue"), but it should be fixed. Having duplicate test IDs makes it impossible to reliably target specific buttons in E2E tests.

Recommended Fix:

// Line 96 - "Later" button
.testTag("SecureWalletLater")

// Line 107 - "Continue" button  
.testTag("SecureWalletContinue")

This would allow E2E tests to:

  • Target the "Later" action specifically with SecureWalletLater
  • Target the "Continue" action with SecureWalletContinue
  • Both buttons remain under the parent SecureWallet container (line 51)

Code Quality

  • ✅ Follows Kotlin and Compose conventions
  • ✅ No performance concerns
  • ✅ No security issues
  • ✅ Changes are minimal and focused

Test Coverage

  • ⚠️ No unit/integration tests for this UI component (though E2E tests exist)
  • The PR relies on manual QA and E2E tests for validation

Recommendation

Request Changes: Please fix the duplicate test ID issue before merging. Consider updating the related iOS and E2E test PRs to account for the distinct button identifiers if they expect different test IDs for the two buttons.

Copilot finished reviewing on behalf of piotr-iohk November 24, 2025 12:11
@jvsena42 jvsena42 merged commit 7af4c17 into master Nov 24, 2025
20 checks passed
@jvsena42 jvsena42 deleted the test/ios-tags branch November 24, 2025 12:47
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.

3 participants