Skip to content

tests: add integration test platform - #1450

Open
quexten wants to merge 7 commits into
mainfrom
km/integration-model-server
Open

tests: add integration test platform#1450
quexten wants to merge 7 commits into
mainfrom
km/integration-model-server

Conversation

@quexten

@quexten quexten commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

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

Adds a better platform for integration tests. Essentially, by emulating state + server, we get very simple to reason about and write integration tests.

Please note, the server emulation was the simplest way to get a server in place. The maintenance effort looks fairly low (especially if assisted by claude; we don't care particularly much about the code style / quality of an emulation server that is not in prod), but if in the future we decide that it is high, we can always run it against a real bitwarden-lite instance. THIS IS FUTURE WORK. However, this does mean that tests are not allowed to assert on server state.

We want to make sure to get the test shape right first, and make writing good, useful tests good first before spending effort to spin up integration testing infrastructure.

Example:

  test.each([
    { name: "pbkdf2", kdf: NEW_PBKDF2 },
    { name: "argon2id", kdf: NEW_ARGON2 },
  ])(
    "change kdf to $name",
    async ({ kdf }) => {
      const userKey = await passwordManagerClient.crypto().get_user_encryption_key();

      // 1. Change the KDF
      await passwordManagerClient.user_crypto_management().change_kdf(TEST_PASSWORD, kdf);

      // 2. Verify local state is fine: Lock & unlock
      await client.lock();
      await client.unlock(TEST_PASSWORD);
      const lockUnlockSdk = client.getPasswordManagerClient();

      // 3. Verify server state is fine: Sync from new cliend and unlock
      const reloginClient = harness.newClientEmulator();
      await reloginClient.login(email);
      await reloginClient.unlock(TEST_PASSWORD);
      const reloginSdk = reloginClient.getPasswordManagerClient();

      // 4. Verify the server has the new KDF values
      expect(await client.bridge.get_kdf_config()).toEqual(kdf);

      // 5. Verify the encryption key has not changed
      expect(await lockUnlockSdk.crypto().get_user_encryption_key()).toBe(userKey);
      expect(await reloginSdk.crypto().get_user_encryption_key()).toBe(userKey);
    },
    TIMEOUT,
  );

The setup for the test is the following:

  beforeEach(async () => {
    harness = testHarness();
    email = harness.server.seedUser(MASTER_PASSWORD_ACCOUNT).email;

    client = harness.newClientEmulator();
    await client.login(email);
    await client.unlock(TEST_PASSWORD);
    passwordManagerClient = client.getPasswordManagerClient();
  }, TIMEOUT);

These are very understandable, easy to reason about and easy to extend, compared to the unit tests we currently have.

Moving forward, the plan is to move all integration tests to this kind of model. Further, we will expand this to work on a wide array of test vectors of different cryptographic versions. This will help ensure we catch two clases of bugs that recently slipped by:

  1. Weird format versions in flows that are otherwise tested
  2. Weird behaviors that you don't catch in mocked unit tests with mocked static APIs

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Claude Code validation

Result: Pass

Validated the one Claude-material path in this PR — .claude/rules/integration-tests.md, a new
path-scoped rules file — read from the .claude-pr/ snapshot of the PR's own content. No
credentials, no permission or tool-grant changes, and no attempt to direct the review. Three
findings below are guidance-accuracy issues: the file's worked example and one of its bans name
APIs and files that do not exist in the suite it governs.

Critical

None.

Major

  • .claude/rules/integration-tests.md:52 — The canonical "Arrange, act, assert" example calls
    arrange(account) and destructures assertAccountIntact from it (also used at line 58). Neither
    symbol exists anywhere in the repository — a repo-wide grep for assertAccountIntact and
    arrange( matches only this rules file. The helpers that do exist in
    crates/bitwarden-wasm-internal/integration-tests/tests/model-server/validate.ts are
    validateLocalState, validateAfterLockUnlock, and validateAfterLogoutLogin, all taking
    (local, method, expected, options) with the options.expect override the section goes on to
    describe. This also puts the section in tension with lines 29–36, which show the two-call
    lock/unlock-then-logout/login shape as mandatory. An agent following the example writes code that
    does not compile, or invents the helper. Fix: rewrite the example against the real helpers, or
    land arrange / assertAccountIntact in validate.ts in this PR and keep the example as-is.
  • .claude/rules/integration-tests.md:13 — "Never inspect a request body. No bodyFor, no
    request.body" is stated absolutely, with exceptions granted at line 24 only to
    conformance.test.ts and tests/model-server/model-server.test.ts. bodyFor is exported from
    tests/http-mock.ts and used at 13 call sites across three checked-in files inside this rule's
    path scope — tests/organizations/invite-link.test.ts (9 sites), tests/user-crypto-management/key-id-backfill.test.ts,
    and tests/user-crypto-management/change-kdf.test.ts — and invite-link.test.ts and
    http-mock.ts are both touched by this same PR. An agent editing those files reads a rule the
    surrounding code violates, and will either rewrite working tests unbidden or learn to disregard
    the rule. Fix: scope the ban to new tests, or name those three files as legacy pending
    migration, alongside the existing exception list.

Minor

  • .claude/rules/integration-tests.md:24 — Names conformance.test.ts as an exception, but no such
    file exists under crates/bitwarden-wasm-internal/integration-tests/. The category is defined as
    a planned test kind in the crate's Readme.md:62 (added by this same PR), so the reference is
    deliberate rather than a typo, but the exception is currently unreachable. Fix: mark it as a
    planned category ("conformance.test.ts, once it exists"), or drop it until the file lands.

Notes

  • The file's YAML frontmatter parses and its paths: key matches the four sibling rules
    (bindings.md, crypto.md, generated-api-crates.md, rust-conventions.md).
  • Its scope overlaps bindings.md (crates/bitwarden-wasm-internal/**), so both rules load for
    these files. That appears intentional and is not a finding.
  • Other API claims were checked and hold: secretLeaks(), seedUser (returns SeededAccount,
    which carries email), LocalState.seedAccount / seedVault, syncToLocalState,
    validateLocalState, options.expect, the asCipherId / asUserId / asEncString helpers in
    tests/type-assertion-helpers.ts, tests/model-server/dto.ts, and npm test running
    tsc --noEmit first (package.json:7).

Checks run

Check Status
Plugin structure Skipped — run by the workflow as a dedicated step, not by this review; also gated off (no changed plugins, no .claude-plugin/marketplace.json)
Marketplace Skipped — same: dedicated workflow step, and no marketplace manifest in this repo
Version bump Skipped — same: dedicated workflow step, and no component plugins changed
Plugin validation (AI) Skipped — no plugins/ path changed
Skill review (AI) Skipped — no SKILL.md changed
Configuration & security Passed — 2 major, 1 minor; no secrets, no permission/tool-grant changes, no settings.local.json in the changeset

@quexten
quexten force-pushed the km/integration-model-server branch from a3b94d7 to 2a3a596 Compare September 4, 2026 05:46
@quexten quexten changed the title tests: rebuild the integration test harness tests: add an in-memory model of the API for integration tests Sep 4, 2026
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

🔍 SDK Breaking Change Detection

SDK Version: km/integration-model-server (a3b94d7)

⚠️ If breaking changes are detected, a corresponding pull request addressing them must be ready for merge in the affected client repository.

Client Status Details
typescript ✅ No breaking changes detected Compilation passed with new SDK version - View Details
android ✅ No breaking changes detected Compilation passed with new SDK version - View Details

Breaking change detection uses the build of the SDK from this branch, including any incompatibities pre-existing on or merged into this branch. Check the workflow logs to confirm.
Results update as workflows complete.

@quexten
quexten force-pushed the km/integration-model-server branch from 2a3a596 to 6178c03 Compare September 8, 2026 13:05
masterKey: null,
privateKey: String(V2_PRIVATE_KEY),
publicKey: "cHVibGljLWtleQ==",
verifyingKey: "dmVyaWZ5aW5nLWtleQ==",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Exposed secret in crates/bitwarden-wasm-internal/integration-tests/vectors/accounts.ts - low severity
Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

Reply @AikidoSec ignore: [REASON] to ignore this issue.
More Info

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.

@AikidoSec ignore: Test vector

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Based on your feedback, we ignored this issue because of the following reason:

Test vector

@quexten
quexten requested a review from eliykat September 8, 2026 13:08
@quexten quexten added the ai-review Request a Claude code review label Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Re-reviewed the integration-test platform after the latest round: the server emulator (api-server, database, dto, entities, authentication, http-mock, key-connector-server, identity-server), the client emulator and its local state, the harness, the account vectors, and the ported change-kdf and key-id-backfill suites plus the type-assertion and route-import updates to the existing invite-link and policy suites. Everything in this diff is test-only, under crates/bitwarden-wasm-internal/integration-tests/; no production code, no dependency manifest changes, and the package.json edit only adds typecheck and build:test scripts while tsconfig.json widens include so the emulator and vector directories are now type-checked. The two new commits hold up: lock() now replaces the handle with a freshly locked client so a post-lock getPasswordManagerClient() can no longer decrypt, and the KDF write on the sync path is scoped to accounts with no master-password unlock data, so the change-kdf assertions at lines 90 and 149 read what the SDK's own sync handler persisted rather than a value the emulator wrote back. POST /ciphers/create now parses the nested CipherCreateRequestModel shape and passes collectionIds through, matching create_cipher's two branches.

Code Review Details
  • ♻️ : Comment above the KDF write inverts the sync handler's behavior (already raised, thread still open)
    • client-emulator/client-emulator.ts:58

Comment thread crates/bitwarden-wasm-internal/integration-tests/test-harness.ts
@quexten
quexten requested a review from mzieniukbw September 8, 2026 13:19
@quexten quexten changed the title tests: add an in-memory model of the API for integration tests tests: add integration test platform Sep 8, 2026
@codecov

codecov Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.23%. Comparing base (26112cf) to head (a64a237).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1450   +/-   ##
=======================================
  Coverage   86.23%   86.23%           
=======================================
  Files         555      555           
  Lines       80085    80085           
=======================================
  Hits        69060    69060           
  Misses      11025    11025           

☔ View full report in Codecov by Harness.
📢 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread crates/bitwarden-wasm-internal/integration-tests/server-emulator/api-server.ts Outdated
@eliykat
eliykat removed their request for review September 9, 2026 05:58

@eliykat eliykat left a comment

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.

Reviewed at a high level and discussed in Slack :) Thank you for working on testing utils!

/**
* Simulates a sync from server to client
*/
async sync(email: string): Promise<void> {

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.

This will eventually be moved entirely into the sdk once sdk gains sync.

@quexten
quexten marked this pull request as ready for review September 9, 2026 15:41
@quexten
quexten requested review from a team as code owners September 9, 2026 15:41
Comment on lines +58 to +59
// Quirk, the crypto sync handler writes the kdf only when the account has no master-password
// but clients always write it.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

♻️ DEBT: Finding 1: The comment states the inverse of what the sync handler does, and of what the branch below it does.

Details and fix

crypto_sync_handler.rs:179-186 writes the KDF in the Some(master_password_unlock) arm only — so the handler writes it when the account has master-password unlock data, and writes nothing for an account without one. That is why the branch below fills the gap for masterPasswordUnlock === null, and the code is right. The comment says the opposite, so a reader reconciling it against the if will conclude one of the two is wrong; deleting the branch on that basis leaves a V2/no-master-password account with no KDF in the bridge, and LocalState.unlock then throws "no synced key material".

    // Quirk: the crypto sync handler writes the kdf only for accounts that have master-password
    // unlock data, but real clients read it off the profile and always write it.
    if (user.masterPasswordUnlock === null) {

eliykat
eliykat previously approved these changes Sep 10, 2026

@eliykat eliykat left a comment

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.

AC changes ✅

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants