-
Notifications
You must be signed in to change notification settings - Fork 934
Add Android testing skill for Claude #6370
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6370 +/- ##
=======================================
Coverage 85.77% 85.77%
=======================================
Files 775 775
Lines 56203 56203
Branches 8123 8123
=======================================
Hits 48207 48207
Misses 5172 5172
Partials 2824 2824 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Great job! No new security vulnerabilities introduced in this pull request |
ebe7698 to
f83fdfc
Compare
This commit introduces a new Claude skill named `testing-android-code` to provide comprehensive guidance and standardized patterns for writing tests within the Bitwarden Android codebase.
The primary objective is to codify best practices and provide developers with quick access to reference documentation, examples, and common pitfalls, thereby improving test quality, consistency, and development efficiency.
The skill is composed of the following documents:
- A main `SKILL.md` file that serves as a quick-reference guide, outlining key patterns for testing ViewModels, Compose UIs, Repositories, and Services. It also introduces test data builders and utility classes.
- A `README.md` explaining the purpose and contents of the skill.
- Detailed reference documents for:
- `test-base-classes.md`: Explains the usage of `BaseViewModelTest`, `BitwardenComposeTest`, and `BaseServiceTest`.
- `flow-testing-patterns.md`: Details how to test StateFlow and EventFlow using Turbine, highlighting the critical differences in their testing patterns.
- `critical-gotchas.md`: Lists common antipatterns and mistakes, such as misusing `assertCoroutineThrows` with `runTest` and forgetting to unmock static functions.
- Complete, copy-pasteable examples for testing major components:
- `viewmodel-test-example.md`
- `compose-screen-test-example.md`
- `repository-test-example.md`
f83fdfc to
db99690
Compare
|
https://bitwarden.slack.com/archives/C09MHQNCSKD/p1769693094966979 -- is this a skill or a prompt? And do we think this will move to the marketplace in time or is so specific it will just be here? I am thinking about the reviewer agent benefitting. |
|
@withinfocus it's a Skill, although I'm toying with the idea of making it a Rule once I've had a chance to tinker with them. I keep going back-and-forth on moving it to the marketplace or keeping it here given it's so specialized. My main hesitation is the friction it may introduce for maintenance. If we consider it a living document to update as we fine-tune Claude for this repo, I would prefer it reside here. I feel like that will encourage and facilitate faster iterations. The reviewer agent, and the implementor/developer agent being built, will inherently have access to this as long as they're initiated in the repo root. If we move it to the marketplace the plugin has to be manually installed everywhere. There may be ways to configure the repo to automagically install the marketplace+plugin, but I've not looked into that approach thoroughly. For the sake of getting something started I think it's best we begin with it here, iterate a bit until the team is confident with it, then promote it to the marketplace if there's a larger desire/need to do so. Edit: |
| } | ||
|
|
||
| companion object { | ||
| private val DEFAULT_STATE = ExampleState( |
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.
Can we move this out of the companion object and just have it be at the top-level of the file?
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.
Done
| } | ||
|
|
||
| // Default state for assertions - always define complete expected states | ||
| private val defaultState = ExampleState( |
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.
Stuff like this is usually top-level
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.
..and done.
| private fun createViewModel( | ||
| initialState: ExampleState? = null, | ||
| ): ExampleViewModel = ExampleViewModel( | ||
| savedStateHandle = SavedStateHandle().apply { set("state", initialState) }, |
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.
👍
Moves the `DEFAULT_STATE` constant from a class-level property to a top-level private val in both `viewmodel-test-example.md` and `compose-screen-test-example.md`. This change improves consistency and cleans up the test class structure by removing the `companion object` in the Compose test example.
|
Claude finished @SaintPatrck's task in 1m 40s —— View job Code Review CompleteThis PR introduces a comprehensive testing skill for Claude that codifies Bitwarden Android testing patterns. Overall, the documentation is well-structured and accurately reflects the codebase patterns. SummaryThe skill documentation provides excellent coverage of:
The examples are thorough and demonstrate real-world testing scenarios. Issues Found1. Incorrect file path for The documentation states the location as:
But the actual location is:
This also affects the SKILL.md file at line 204 which lists this path. Review Comments Already AddressedThe previous review comments have been addressed:
Outstanding Review CommentRegarding repository-test-example.md: @david-livefront asked about the Minor Suggestions
VerificationI verified the following patterns against the actual codebase:
RecommendationApprove with minor changes - The path for |
|
|
||
| /** | ||
| * Test: Back button sends action to ViewModel | ||
| */ |
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.
We generally don't have docs on our tests, right?
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.
No, this is so Claude has examples of associating what is being tested to expected test names. Claude is not actually writing KDoc like this.
.claude/skills/testing-android-code/examples/repository-test-example.md
Outdated
Show resolved
Hide resolved
|
|
||
| /** | ||
| * PATTERN: For error paths that require exceptions, create isolated | ||
| * repository instances with mocked dependencies. |
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.
Do we ever do this?
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.
No. It's coming from #6424. I'm going to remove it since we don't encounter this scenario often.
- Compose tests: Change @beforeeach to @before (JUnit 4) - Compose tests: Add dialog testing examples with isDialog() - Repository tests: Fix Fake pattern to use private storage with setter - Repository tests: Remove fictional emitData() helper method - SKILL.md: Remove PR review references from pattern descriptions Co-Authored-By: Claude Opus 4.5 <[email protected]>
Rename "Reference Implementations" to "Complete Examples" and update paths to point to the local example files instead of codebase files. Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
These latest changes are looking good. #6442 was generated after the latest updates and correctly follows our testing guidelines for View Models. 🥳 |

🎟️ Tracking
TBD
📔 Objective
Introduce a new Claude skill named
testing-android-codeto provide comprehensive guidance and standardized patterns for writing tests within the Bitwarden Android codebase.The primary objective is to codify best practices and provide Claude with quick access to reference documentation, examples, and common pitfalls, thereby improving test quality, consistency, and development efficiency.
The skill is composed of the following documents:
SKILL.mdfile that serves as a quick-reference guide, outlining key patterns for testing ViewModels, Compose UIs, Repositories, and Services. It also introduces test data builders and utility classes.README.mdexplaining the purpose and contents of the skill.test-base-classes.md: Explains the usage ofBaseViewModelTest,BitwardenComposeTest, andBaseServiceTest.flow-testing-patterns.md: Details how to test StateFlow and EventFlow using Turbine, highlighting the critical differences in their testing patterns.critical-gotchas.md: Lists common antipatterns and mistakes, such as misusingassertCoroutineThrowswithrunTestand forgetting to unmock static functions.viewmodel-test-example.mdcompose-screen-test-example.mdrepository-test-example.md⏰ Reminders before review
🦮 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