Skip to content

[PM-33574] llm: Add build-test-verify skill#2448

Merged
SaintPatrck merged 2 commits intomainfrom
llm/build-test-verify
Mar 30, 2026
Merged

[PM-33574] llm: Add build-test-verify skill#2448
SaintPatrck merged 2 commits intomainfrom
llm/build-test-verify

Conversation

@SaintPatrck
Copy link
Copy Markdown
Contributor

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-33574

📔 Objective

Consolidates all build, test, lint, and troubleshooting content from CLAUDE.md into a dedicated on-demand skill.

What changed:

  • skills/build-test-verify/SKILL.md — Complete reference for: initial setup (bootstrap.sh), project generation (XcodeGen specs), build commands, test execution with correct simulator requirements, lint/format/spell check, Sourcery mock generation, SwiftGen asset generation, tooling reference table, common failures table (missing .xcodeproj, missing mocks, snapshot failures, extension memory crashes, SwiftLint TODO warning), and debug tips.

  • CLAUDE.md — Removes Pre-build Scripts subsection, the entire Deployment section (building, initial setup, git hooks, code generation, CI/CD, tooling table), and the entire Troubleshooting section. Replaces ~100 lines with a two-line pointer to the build-test-verify skill.

@github-actions github-actions bot added the t:llm Change Type - LLM related change (e.g. CLAUDE.md files) label Mar 13, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 13, 2026

Logo
Checkmarx One – Scan Summary & Details3bd82475-e52c-455a-a02e-dcc92867598c

Great job! No new security vulnerabilities introduced in this pull request

@SaintPatrck SaintPatrck added the ai-review Request a Claude code review label Mar 13, 2026
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Mar 13, 2026

Claude finished @SaintPatrck's task in 2m 24s —— View job


Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the extraction of build, test, lint, and troubleshooting content from CLAUDE.md into a dedicated build-test-verify skill file. The latest commit addresses all five prior reviewer threads: Test Harness build example added, snapshot re-record section replaced with "currently disabled" guidance, swiftgen-bwr.yml added as the primary SwiftGen config alongside all other configs, and typos added to the tooling reference table. The migration is faithful to the original content with valid improvements.

No findings.

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Mar 13, 2026

Overall Assessment: APPROVE

Reviewed the extraction of build/test/troubleshooting content from CLAUDE.md into a dedicated build-test-verify skill. The migration is faithful, with improvements including additional Sourcery configs for AuthenticatorShared and BitwardenKit, and a more compact troubleshooting table.

Code Review Details
  • ⚠️ IMPORTANT: Snapshot re-record command is missing -workspace and -scheme flags, making it appear complete but non-runnable
    • .claude/skills/build-test-verify/SKILL.md:55

Finding 1 -- .claude/skills/build-test-verify/SKILL.md:55-56

⚠️ IMPORTANT: Snapshot re-record command is missing -workspace and -scheme flags

Details and fix

The regular test command (line 42) correctly includes -workspace Bitwarden.xcworkspace -scheme Bitwarden, but the snapshot re-record command at line 55 omits them. Without these flags, xcodebuild test cannot resolve the test plan and will fail.

The source in Docs/Testing.md:697 uses ... to indicate truncation, but in SKILL.md the trailing ... was replaced with a specific -destination flag, making the command appear complete and ready to execute when it is not.

Suggested fix:

RECORD_MODE=1 xcodebuild test \
  -workspace Bitwarden.xcworkspace \
  -scheme Bitwarden \
  -testPlan Bitwarden-Snapshot \
  -destination 'platform=iOS Simulator,name=iPhone 16'

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 13, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.80%. Comparing base (48115b8) to head (a5624ec).
⚠️ Report is 43 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2448      +/-   ##
==========================================
- Coverage   86.86%   85.80%   -1.07%     
==========================================
  Files        1841     2076     +235     
  Lines      162244   178294   +16050     
==========================================
+ Hits       140941   152987   +12046     
- Misses      21303    25307    +4004     

☔ 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.

@SaintPatrck SaintPatrck marked this pull request as ready for review March 18, 2026 00:59
@SaintPatrck SaintPatrck requested a review from a team as a code owner March 18, 2026 00:59
@SaintPatrck SaintPatrck requested review from a team and matt-livefront March 18, 2026 00:59
@SaintPatrck SaintPatrck force-pushed the llm/build-test-verify branch from ce11fe5 to 008392a Compare March 19, 2026 16:23
theMickster
theMickster previously approved these changes Mar 20, 2026
Copy link
Copy Markdown
Member

@fedemkr fedemkr left a comment

Choose a reason for hiding this comment

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

Looks good, just some improvements. Let me know what you think and I can do the changes, no problem.

Comment on lines +31 to +37
## Building

```bash
./Scripts/build.sh project-pm.yml Bitwarden Simulator # PM for simulator
./Scripts/build.sh project-bwa.yml Authenticator Simulator # Authenticator for simulator
./Scripts/build.sh project-pm.yml Bitwarden Device # PM for device
```
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤔 Maybe we should do some testing on this, but is this smart / fast enough on detecting that to build Test Harness for device it needs to use ./Scripts/build.sh project-bwth.yml TestHarness Device? Should the general script be added here? Although maybe it just fetches the docs from build.sh so this may not be necessary.

./Scripts/build.sh project-{productAcronym}.yml {build_scheme} {build_mode}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We can add an example of building the test harness to save Claude a few extra tool calls.

Comment on lines +53 to +60
**Re-record snapshot tests** (when visual changes are intentional):
```bash
RECORD_MODE=1 xcodebuild test \
-workspace Bitwarden.xcworkspace \
-scheme Bitwarden \
-testPlan Bitwarden-Snapshot \
-destination 'platform=iOS Simulator,name=iPhone 16'
```
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

⚠️ I think we should let Claude know Snapshot tests are disabled for the time being so it doesn't try to re-record snapshot tests.

mint run sourcery --config BitwardenKit/Sourcery/sourcery.yml

# Asset/localization code generation
mint run swiftgen config run --config swiftgen-pm.yml
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

⛏️ I'd use swiftgen-bwr.yml instead here as it's the most likely place to need asset/localization generation given that a lot of them are shared now in "Bitwarden Resources".

Comment on lines +87 to +97
## Tooling Reference

| Tool | Config | Purpose |
|------|--------|---------|
| XcodeGen | `project-*.yml` | Generates `.xcodeproj` from YAML specs |
| Mint | `Mintfile` | Swift tool package manager |
| SwiftLint | `.swiftlint.yml` | Linting with custom rules |
| SwiftFormat | `.swiftformat` | Code formatting |
| Sourcery | `*/Sourcery/sourcery.yml` | Mock generation (`AutoMockable`) |
| SwiftGen | `swiftgen-*.yml` | Asset/localization code generation |
| Fastlane | `fastlane/Fastfile` | CI/CD automation |
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤔 Should typos be included here as well?

|---------|-------|-----|
| `.xcodeproj` not found | Files are gitignored | Run `./Scripts/bootstrap.sh` |
| `MockXxx` not found | Sourcery not run | Add `// sourcery: AutoMockable`, run Sourcery or build |
| Snapshot test fails | Simulator mismatch or intentional change | Check `.test-simulator-*` files; re-record if change is intentional |
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤔 I'd indicate that these are disabled for the time being and if any snapshot test is run then it should actually disable it by adding disable to the test function so it's transformed to something like disabletest_snapshot_*.

- Add Test Harness build example
- Note snapshot tests are currently disabled
- List all SwiftGen configs with BitwardenResources as primary
- Add typos to tooling reference table
- Update snapshot failure row to reflect disabled status

CI runs all `-Default` test plans on PRs to `main`, commits to `main`, and release branches. Test execution order is randomized (`randomExecutionOrder: true`).

**Snapshot tests are currently disabled.** Do not run or re-record them. If you encounter a snapshot test, prefix the function name with `disable` (e.g., `disabletest_snapshot_defaultState`).
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@fedemkr how does this sound?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Perfect, thanks!

@SaintPatrck SaintPatrck merged commit da3e555 into main Mar 30, 2026
30 of 31 checks passed
@SaintPatrck SaintPatrck deleted the llm/build-test-verify branch March 30, 2026 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review t:llm Change Type - LLM related change (e.g. CLAUDE.md files)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants