Conversation
There was a problem hiding this comment.
Pull request overview
Adds end-to-end (E2E-style) test scaffolding intended to validate SD card flashing in CI, plus CI/Makefile wiring to run these tests.
Changes:
- Added new Rust E2E-style test modules under
tests/e2e/for SD flashing and formatting. - Added Makefile targets and GitHub Actions workflows intended to run E2E tests across OSes.
- Minor refactor in
bb-downloaderto avoidlet-chain usage.
Reviewed changes
Copilot reviewed 6 out of 13 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
tests/e2e/sd.rs |
Adds SD flashing/formatting E2E tests targeting a “virtual SD card” path. |
tests/e2e/mod.rs |
Adds shared helpers for creating/cleaning temp images/devices for E2E tests. |
bb-downloader/src/lib.rs |
Refactors SHA-256 cache hit logic to nested if statements. |
Makefile |
Adds test-e2e* targets intended to run E2E integration tests. |
Cargo.toml |
Adds a comment about removing the e2e-tests crate in favor of tests/e2e. |
.github/workflows/e2e.yml |
Adds an E2E workflow matrix for SD/BCF/DFU testing. |
.github/workflows/e2e-tests.yml |
Adds a second E2E workflow that runs tests from an e2e-tests/ directory. |
.gitignore |
Normalizes .DS_Store line and adds .idea/ ignore. |
.idea/* |
Adds IntelliJ project configuration files. |
Files not reviewed (6)
- .idea/.gitignore: Language not supported
- .idea/copilot.data.migration.ask2agent.xml: Language not supported
- .idea/inspectionProfiles/Project_Default.xml: Language not supported
- .idea/inspectionProfiles/profiles_settings.xml: Language not supported
- .idea/misc.xml: Language not supported
- .idea/vcs.xml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let sd_path = create_virtual_sd_card(SD_SIZE).expect("failed to create virtual SD card"); | ||
|
|
||
| let img = bb_flasher::LocalImage::new(img_path.clone().into_boxed_path()); | ||
| let target: bb_flasher::sd::Target = sd_path.clone().try_into().expect("failed to create SD target"); | ||
|
|
There was a problem hiding this comment.
bb_flasher::sd::Target’s TryFrom<PathBuf> only succeeds if the path is present in bb_flasher_sd::devices() (removable, non-virtual drives). A temp file created by create_virtual_sd_card will never appear there, so this test will fail before flashing starts. Consider adding an explicit “file-backed target”/test-only constructor, or call into an API that accepts an arbitrary path for test destinations.
| let target: bb_flasher::sd::Target = sd_path.clone().try_into().expect("failed to create SD target"); | ||
|
|
||
| let formatter = bb_flasher::sd::Formatter::new(target, None); | ||
|
|
||
| let result = formatter.format(None).await; |
There was a problem hiding this comment.
Formatter::format is implemented to format a real device/volume (and on Windows uses volume locking); passing a regular temp file path is unlikely to work cross-platform. Either gate this test to supported platforms/implementations, or add a test-only formatting path that operates on file-backed images.
| //! Shared helpers for E2E-style integration tests. | ||
|
|
There was a problem hiding this comment.
The workspace root Cargo.toml is a virtual manifest (no [package]), so integration tests under the repository-level tests/ directory won’t be built or run by cargo test. These E2E tests need to live under a real crate (e.g., bb-flasher/tests/...) or you need to add a dedicated e2e-tests crate back into the workspace.
| - 'bb-flasher-pb2-mspm0/**' | ||
| - 'e2e-tests/**' | ||
| - '.github/workflows/e2e.yml' | ||
| push: | ||
| branches: [ "main" ] | ||
| paths: | ||
| - 'bb-flasher/**' | ||
| - 'bb-flasher-sd/**' | ||
| - 'bb-flasher-bcf/**' | ||
| - 'bb-flasher-dfu/**' | ||
| - 'bb-flasher-pb2-mspm0/**' | ||
| - 'e2e-tests/**' | ||
| - '.github/workflows/e2e.yml' | ||
| workflow_dispatch: | ||
|
|
||
| env: | ||
| CARGO_TERM_COLOR: always | ||
| RUST_BACKTRACE: 1 | ||
|
|
||
| jobs: | ||
| e2e-sd: | ||
| name: E2E Tests - SD Flashing | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| os: [ubuntu-latest, windows-latest, macos-latest] | ||
| runs-on: ${{ matrix.os }} | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v6 | ||
| with: | ||
| lfs: true | ||
|
|
||
| - name: Install dependencies (Ubuntu) | ||
| if: matrix.os == 'ubuntu-latest' | ||
| run: make setup-debian-deps | ||
|
|
||
| - name: Select rust toolchain | ||
| run: rustup toolchain install stable --profile minimal | ||
|
|
||
| - name: Use caching | ||
| uses: Swatinem/rust-cache@v2 | ||
| with: | ||
| cache-on-failure: true | ||
|
|
||
| - name: Run SD E2E tests | ||
| run: cargo test -p e2e-tests sd_flash --features sd -- --test-threads=1 | ||
| continue-on-error: ${{ matrix.os == 'windows-latest' }} |
There was a problem hiding this comment.
This workflow references an e2e-tests crate (cargo test -p e2e-tests ...) and e2e-tests/** paths, but there is no e2e-tests directory/package in the repo and the workspace manifest comment says it was removed. Update the workflow to run the actual test target/package that contains the new E2E tests (or reintroduce the e2e-tests crate), and update the paths: filters accordingly.
| paths: | ||
| - 'e2e-tests/**' | ||
| - 'bb-flasher/**' | ||
| - 'bb-flasher-*/**' | ||
| - '.github/workflows/e2e-tests.yml' | ||
| pull_request: | ||
| branches: [ main, develop ] | ||
| paths: | ||
| - 'e2e-tests/**' | ||
| - 'bb-flasher/**' | ||
| - 'bb-flasher-*/**' | ||
|
|
||
| env: | ||
| CARGO_TERM_COLOR: always | ||
| RUST_BACKTRACE: 1 | ||
|
|
||
| jobs: | ||
| # ================================ | ||
| # Linux E2E Tests | ||
| # ================================ | ||
| e2e-linux: | ||
| name: E2E Tests (Linux) | ||
| runs-on: ubuntu-latest | ||
|
|
||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| test-suite: | ||
| - name: SD Card | ||
| features: sd | ||
| - name: BCF | ||
| features: bcf,bcf_msp430 | ||
| - name: DFU | ||
| features: dfu | ||
|
|
||
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Install system dependencies | ||
| run: | | ||
| sudo apt-get update | ||
| sudo apt-get install -y \ | ||
| libudev-dev \ | ||
| libusb-1.0-0-dev \ | ||
| pkg-config | ||
|
|
||
| - name: Install Rust toolchain | ||
| uses: actions-rs/toolchain@v1 | ||
| with: | ||
| toolchain: stable | ||
| profile: minimal | ||
| override: true | ||
|
|
||
| - name: Cache cargo registry | ||
| uses: actions/cache@v3 | ||
| with: | ||
| path: ~/.cargo/registry | ||
| key: ${{ runner.os }}-cargo-registry-${{ hashFiles('**/Cargo.lock') }} | ||
|
|
||
| - name: Cache cargo index | ||
| uses: actions/cache@v3 | ||
| with: | ||
| path: ~/.cargo/git | ||
| key: ${{ runner.os }}-cargo-index-${{ hashFiles('**/Cargo.lock') }} | ||
|
|
||
| - name: Cache cargo build | ||
| uses: actions/cache@v3 | ||
| with: | ||
| path: target | ||
| key: ${{ runner.os }}-cargo-build-target-${{ hashFiles('**/Cargo.lock') }} | ||
|
|
||
| - name: Run ${{ matrix.test-suite.name }} tests | ||
| run: | | ||
| cd e2e-tests | ||
| cargo test --test e2e --features ${{ matrix.test-suite.features }} -- --nocapture | ||
|
|
There was a problem hiding this comment.
This workflow cd e2e-tests / triggers on e2e-tests/**, but that directory doesn’t exist in the repository. Either remove this workflow in favor of a single E2E workflow, or update it to run the E2E tests from the correct crate/path (e.g., bb-flasher/tests/...) and adjust triggers accordingly.
| pub fn create_test_image(size: usize) -> std::io::Result<PathBuf> { | ||
| let temp_dir = std::env::temp_dir(); | ||
| let img_path = temp_dir.join(format!("test_image_{}.img", uuid::Uuid::new_v4())); | ||
|
|
||
| let mut file = File::create(&img_path)?; |
There was a problem hiding this comment.
This uses uuid::Uuid::new_v4(), but there is no uuid dependency in the repo’s Cargo manifests. Add uuid as a dev-dependency in the crate that owns these tests (or switch to tempfile to avoid needing UUIDs).
| $(CARGO_PATH) test --tests -- --test-threads=1 e2e:: | ||
|
|
||
| ## housekeeping: test-e2e-sd: Run E2E-style tests for SD card flashing | ||
| .PHONY: test-e2e-sd | ||
| test-e2e-sd: | ||
| $(info "Run SD card E2E-style tests") | ||
| $(CARGO_PATH) test --tests -- --test-threads=1 e2e::sd | ||
|
|
||
| ## housekeeping: test-e2e-bcf: Run E2E-style tests for BCF flashing | ||
| .PHONY: test-e2e-bcf | ||
| test-e2e-bcf: | ||
| $(info "Run BCF E2E-style tests") | ||
| $(CARGO_PATH) test --tests -- --test-threads=1 e2e::bcf | ||
|
|
||
| ## housekeeping: test-e2e-dfu: Run E2E-style tests for DFU flashing | ||
| .PHONY: test-e2e-dfu | ||
| test-e2e-dfu: | ||
| $(info "Run DFU E2E-style tests") | ||
| $(CARGO_PATH) test --tests -- --test-threads=1 e2e::dfu |
There was a problem hiding this comment.
These targets invoke cargo test from the workspace root, but the workspace is a virtual manifest and won’t run repository-level tests/e2e (and the e2e::... filter is unlikely to match how Rust test names are reported). Point these commands at an actual package/test target (e.g., cargo test -p bb-flasher --test e2e ...) and pass any required feature flags (--features sd, etc.).
| $(CARGO_PATH) test --tests -- --test-threads=1 e2e:: | |
| ## housekeeping: test-e2e-sd: Run E2E-style tests for SD card flashing | |
| .PHONY: test-e2e-sd | |
| test-e2e-sd: | |
| $(info "Run SD card E2E-style tests") | |
| $(CARGO_PATH) test --tests -- --test-threads=1 e2e::sd | |
| ## housekeeping: test-e2e-bcf: Run E2E-style tests for BCF flashing | |
| .PHONY: test-e2e-bcf | |
| test-e2e-bcf: | |
| $(info "Run BCF E2E-style tests") | |
| $(CARGO_PATH) test --tests -- --test-threads=1 e2e::bcf | |
| ## housekeeping: test-e2e-dfu: Run E2E-style tests for DFU flashing | |
| .PHONY: test-e2e-dfu | |
| test-e2e-dfu: | |
| $(info "Run DFU E2E-style tests") | |
| $(CARGO_PATH) test --tests -- --test-threads=1 e2e::dfu | |
| $(CARGO_PATH) test -p bb-imager-cli --test e2e -- --test-threads=1 | |
| ## housekeeping: test-e2e-sd: Run E2E-style tests for SD card flashing | |
| .PHONY: test-e2e-sd | |
| test-e2e-sd: | |
| $(info "Run SD card E2E-style tests") | |
| $(CARGO_PATH) test -p bb-imager-cli --test e2e --features sd -- --test-threads=1 | |
| ## housekeeping: test-e2e-bcf: Run E2E-style tests for BCF flashing | |
| .PHONY: test-e2e-bcf | |
| test-e2e-bcf: | |
| $(info "Run BCF E2E-style tests") | |
| $(CARGO_PATH) test -p bb-imager-cli --test e2e --features bcf -- --test-threads=1 | |
| ## housekeeping: test-e2e-dfu: Run E2E-style tests for DFU flashing | |
| .PHONY: test-e2e-dfu | |
| test-e2e-dfu: | |
| $(info "Run DFU E2E-style tests") | |
| $(CARGO_PATH) test -p bb-imager-cli --test e2e --features dfu -- --test-threads=1 |
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <project version="4"> | ||
| <component name="VcsDirectoryMappings"> | ||
| <mapping directory="" vcs="Git" /> | ||
| </component> | ||
| </project> No newline at end of file |
There was a problem hiding this comment.
IDE project files under .idea/ generally shouldn’t be committed (and this PR also adds .idea/ to .gitignore). Remove these .idea files from the PR and keep them local to the developer environment.
| <?xml version="1.0" encoding="UTF-8"?> | |
| <project version="4"> | |
| <component name="VcsDirectoryMappings"> | |
| <mapping directory="" vcs="Git" /> | |
| </component> | |
| </project> | |
| <!-- Intentionally left blank: IDE-specific configuration files under .idea/ should not be committed. --> |
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <project version="4"> | ||
| <component name="Black"> | ||
| <option name="sdkName" value="Python 3.12" /> | ||
| </component> | ||
| </project> No newline at end of file |
There was a problem hiding this comment.
IDE project files under .idea/ generally shouldn’t be committed (and this PR also adds .idea/ to .gitignore). Remove these .idea files from the PR and keep them local to the developer environment.
| <?xml version="1.0" encoding="UTF-8"?> | |
| <project version="4"> | |
| <component name="Black"> | |
| <option name="sdkName" value="Python 3.12" /> | |
| </component> | |
| </project> |
| # Default ignored files | ||
| /shelf/ | ||
| /workspace.xml | ||
| # Ignored default folder with query files | ||
| /queries/ | ||
| # Datasource local storage ignored files | ||
| /dataSources/ | ||
| /dataSources.local.xml | ||
| # Editor-based HTTP Client requests | ||
| /httpRequests/ |
There was a problem hiding this comment.
Since .idea/ is now ignored, committing .idea/.gitignore (and other .idea contents) is inconsistent and will keep these files tracked. Remove .idea/ from the repository history in this PR (git rm --cached) so the ignore rule takes effect.
| # Default ignored files | |
| /shelf/ | |
| /workspace.xml | |
| # Ignored default folder with query files | |
| /queries/ | |
| # Datasource local storage ignored files | |
| /dataSources/ | |
| /dataSources.local.xml | |
| # Editor-based HTTP Client requests | |
| /httpRequests/ | |
| # Intentionally left empty. The `.idea/` directory is ignored at the repository root. |
| @@ -0,0 +1,47 @@ | |||
| //! Shared helpers for E2E-style integration tests. | |||
There was a problem hiding this comment.
Please first learn how tests in Rust crates are supposed to be structured. You should not create a tests crate like this.
#66 added testing for SD
if approved by @Ayush1325 i can continue with bcf and dfu