Skip to content

Conversation

@shane-melton
Copy link
Member

🎟️ Tracking

N/A

📔 Objective

Several tests referenced fields and methods only available with the wasm feature. Gate these code sections with #[cfg(feature = "wasm")] to allow tests to compile without the wasm feature.

This fixes cargo test --package bitwarden-vault which previously failed due to feature resolution differences between workspace and package-specific testing.

⏰ 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

Several tests reference fields and methods only available with the wasm feature.
Gate these code sections with #[cfg(feature = "wasm")] to allow tests to compile
without the wasm feature.

This fixes cargo test --package bitwarden-vault which previously failed due to
feature resolution differences between workspace and package-specific testing.
Move imports only used in wasm-gated code behind #[cfg(feature = "wasm")] to
eliminate unused import warnings when compiling without the wasm feature.
@shane-melton shane-melton requested review from a team as code owners October 31, 2025 19:59
@claude
Copy link

claude bot commented Oct 31, 2025

Claude finished @shane-melton's task —— View job


Code Review Complete

Summary of Changes

This PR adds proper feature gating for wasm-specific code in the bitwarden-vault crate. The changes fix compilation issues when running cargo test --package bitwarden-vault without the wasm feature enabled by ensuring that:

  1. Wasm-specific field initializations in tests are gated with #[cfg(feature = "wasm")]
  2. Wasm-specific imports are properly feature-gated
  3. A wasm-specific test function is marked with the appropriate feature attribute

Files modified:

  • crates/bitwarden-vault/src/cipher/attachment.rs (3 test instances)
  • crates/bitwarden-vault/src/cipher/cipher.rs (4 test instances)
  • crates/bitwarden-vault/src/cipher/cipher_client/mod.rs (imports and 1 test function)
  • crates/bitwarden-vault/src/collection_client.rs (1 import)

Critical Issues

None identified. The changes are correct and complete.


Suggested Improvements

Consider adding a CI check for feature-gated compilation

To prevent similar issues in the future, consider adding a CI job that tests compilation without optional features:

- name: Test without optional features
  run: cargo test --package bitwarden-vault --no-default-features

This would catch feature gating issues early in development.


Good Practices Observed

  • ✅ Consistent application of #[cfg(feature = "wasm")] across all affected code
  • ✅ Proper feature gating for both struct fields and their usages in tests
  • ✅ Import statements correctly feature-gated alongside their usage
  • ✅ Test function properly marked with feature attribute when it uses wasm-specific types

Detailed Analysis

Feature Gating Pattern

The PR correctly applies the #[cfg(feature = "wasm")] attribute pattern that matches the existing codebase style:

Struct field definition (attachment.rs:60-61):

#[cfg(feature = "wasm")]
pub decrypted_key: Option<String>,

Test field initialization (all test files):

#[cfg(feature = "wasm")]
decrypted_key: None,

This is the correct approach for conditional compilation in Rust.

Import Gating (cipher_client/mod.rs)

The changes properly separate imports by feature:

use bitwarden_crypto::IdentifyKey;
#[cfg(feature = "wasm")]
use bitwarden_crypto::{CompositeEncryptable, SymmetricCryptoKey};

This is more maintainable than:

use bitwarden_crypto::{IdentifyKey, CompositeEncryptable, SymmetricCryptoKey};

Because CompositeEncryptable and SymmetricCryptoKey are only used in wasm-feature-gated code.

Test Function Gating (cipher_client/mod.rs:538-539)

The test_encrypt_cipher_for_rotation test is correctly marked with both:

#[tokio::test]
#[cfg(feature = "wasm")]

This test uses CryptoError which is only imported under the wasm feature, and tests the encrypt_cipher_for_rotation method which is itself wasm-gated.

Coverage Verification

I verified that all 8 instances of AttachmentView initialization in test code are now properly gated:

  • ✅ attachment.rs:290 (test_encrypt_attachment)
  • ✅ attachment.rs:349 (test_attachment_key)
  • ✅ attachment.rs:409 (test_attachment_without_key)
  • ✅ cipher.rs:1093 (test_generate_cipher_key_ignores_attachments_without_key)
  • ✅ cipher.rs:1206 (test_move_user_cipher_with_attachment_without_key_to_org)
  • ✅ cipher.rs:1251 (test_move_user_cipher_with_attachment_with_key_to_org)
  • ✅ cipher.rs:1320 (test_move_user_cipher_with_key_with_attachment_with_key_to_org)
  • ✅ cipher_client/mod.rs: No AttachmentView instantiation in tests affected by wasm imports

Architecture Compliance

Per the CLAUDE.md guidelines, this PR correctly handles feature-gated code without introducing business logic changes. The changes are purely compilation fixes that don't affect runtime behavior when the wasm feature is enabled.


Action Items for Author

None - this PR is ready to merge. The implementation is correct, complete, and follows Rust best practices for conditional compilation.


@github-actions
Copy link
Contributor

github-actions bot commented Oct 31, 2025

Logo
Checkmarx One – Scan Summary & Details15cd202b-672a-4bfa-b654-8ac594e51d12

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

@codecov
Copy link

codecov bot commented Oct 31, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.70%. Comparing base (8ef7951) to head (5f7867d).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #543   +/-   ##
=======================================
  Coverage   78.70%   78.70%           
=======================================
  Files         296      296           
  Lines       30500    30507    +7     
=======================================
+ Hits        24005    24012    +7     
  Misses       6495     6495           

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

@shane-melton shane-melton merged commit 95e329a into main Nov 3, 2025
53 checks passed
@shane-melton shane-melton deleted the vault/fix-wasm-feature-gated-tests branch November 3, 2025 18:24
bw-ghapp bot pushed a commit to bitwarden/sdk-swift that referenced this pull request Nov 3, 2025
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.

4 participants