Conversation
There was a problem hiding this comment.
Pull request overview
Enables stable compilation for Tokio-based doctests (notably #[tokio::main]) in bb-flasher-sd, and introduces initial E2E testing/CI scaffolding across flashers.
Changes:
- Enable Tokio
macros(and refactor a few let-chain conditionals) to improve stable compatibility. - Add SD E2E test scaffolding and Makefile targets for running E2E-style integration tests.
- Add new GitHub Actions workflows intended to run E2E tests across platforms, plus repo ignore/IDE metadata updates.
Reviewed changes
Copilot reviewed 11 out of 19 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
tests/e2e/sd.rs |
Adds SD E2E-style tests targeting a virtual/file-backed device. |
bb-imager-gui/src/helpers.rs |
Refactors locale/keymap parsing logic to avoid let-chains. |
bb-flasher/tests/e2e_sd.rs |
Adds an ignored, destructive SD E2E placeholder test requiring a real device. |
bb-flasher/tests/e2e_common.rs |
Adds shared integration-test helpers for temp image creation/cleanup. |
bb-flasher/Cargo.toml |
Adds dev-deps for Tokio macros and test helper dependencies. |
bb-flasher-sd/src/flashing.rs |
Refactors customization validation to avoid let-chains. |
bb-flasher-sd/Cargo.toml |
Enables Tokio macros for doctests (and aligns Tokio versions across targets). |
bb-downloader/src/lib.rs |
Refactors a conditional to avoid let-chains without changing behavior. |
Makefile |
Adds test-e2e* targets for running E2E-style tests locally. |
Cargo.lock |
Updates lockfile to include new dev-deps and various dependency version bumps. |
.idea/vcs.xml |
Adds IDE configuration (IntelliJ/JetBrains). |
.idea/misc.xml |
Adds IDE configuration (IntelliJ/JetBrains). |
.idea/inspectionProfiles/profiles_settings.xml |
Adds IDE inspection profile settings. |
.idea/inspectionProfiles/Project_Default.xml |
Adds IDE inspection profile definitions. |
.idea/copilot.data.migration.ask2agent.xml |
Adds IDE metadata for Copilot migration state. |
.idea/.gitignore |
Adds IDE-local ignore rules under .idea/. |
.gitignore |
Normalizes .DS_Store entry and adds .idea/ ignore. |
.github/workflows/e2e.yml |
Adds a new E2E workflow intended to run per-flasher E2E suites across OSes. |
.github/workflows/e2e-tests.yml |
Adds an additional E2E workflow matrix across Linux/Windows/macOS. |
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.
| # Needed for #[tokio::main] in doctests | ||
| tokio = { version = "1.49", default-features = false, features = ["rt-multi-thread", "macros"] } |
There was a problem hiding this comment.
The crate uses tokio::sync, tokio::fs, and tokio::task in its library code (e.g. src/flashing.rs, src/pal/*), but the tokio dependency here only enables rt-multi-thread and macros. This relies on workspace feature unification and can break when bb-flasher-sd is built standalone (including doctests). Add the specific Tokio features you actually use (at least sync and fs, plus process/io-util where applicable) instead of only macros.
| - 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 assumes an e2e-tests/ directory and runs cd e2e-tests, but that directory doesn't exist in the repository. Either add the e2e-tests project (and any referenced scripts) or update the workflow to run the actual integration tests that exist (e.g. cargo test -p bb-flasher --tests ...).
| cd e2e-tests | ||
| cargo test --test e2e --features ${{ matrix.test-suite.features }} -- --nocapture |
There was a problem hiding this comment.
This workflow assumes an e2e-tests/ directory and runs cd e2e-tests, but that directory doesn't exist in the repository. Either add the e2e-tests project (and any referenced scripts) or update the workflow to run the actual integration tests that exist (e.g. cargo test -p bb-flasher --tests ...).
| cd e2e-tests | |
| cargo test --test e2e --features ${{ matrix.test-suite.features }} -- --nocapture | |
| cargo test -p bb-flasher --tests --features ${{ matrix.test-suite.features }} -- --nocapture |
| <?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-specific .idea/* configuration is being committed even though the repo ignores .idea/ in .gitignore. These files are typically machine/user-specific and can cause noisy diffs; please remove them from version control (keep them local) and rely on the ignore rule.
| <?xml version="1.0" encoding="UTF-8"?> | |
| <project version="4"> | |
| <component name="VcsDirectoryMappings"> | |
| <mapping directory="" vcs="Git" /> | |
| </component> | |
| </project> |
| cache-on-failure: true | ||
|
|
||
| - name: Run SD E2E tests | ||
| run: cargo test -p e2e-tests sd_flash --features sd -- --test-threads=1 |
There was a problem hiding this comment.
This workflow runs cargo test -p e2e-tests ..., but there is no e2e-tests package/directory in the repo or workspace members, so this step will fail. Point the command at an existing package/tests (e.g. -p bb-flasher --test ...) or add an e2e-tests crate and include it in the workspace.
| run: cargo test -p e2e-tests sd_flash --features sd -- --test-threads=1 | |
| run: cargo test -p bb-flasher --test sd_flash --features sd -- --test-threads=1 |
| cd e2e-tests | ||
| cargo test --test e2e --features ${{ matrix.test-suite.features }} -- --nocapture | ||
|
|
||
| - name: Upload test report | ||
| if: always() | ||
| uses: actions/upload-artifact@v3 | ||
| with: | ||
| name: test-report-macos-${{ matrix.test-suite.features }} | ||
| path: e2e-tests/test-report-*.txt |
There was a problem hiding this comment.
This workflow assumes an e2e-tests/ directory and runs cd e2e-tests, but that directory doesn't exist in the repository. Either add the e2e-tests project (and any referenced scripts) or update the workflow to run the actual integration tests that exist (e.g. cargo test -p bb-flasher --tests ...).
| cd e2e-tests | |
| cargo test --test e2e --features ${{ matrix.test-suite.features }} -- --nocapture | |
| - name: Upload test report | |
| if: always() | |
| uses: actions/upload-artifact@v3 | |
| with: | |
| name: test-report-macos-${{ matrix.test-suite.features }} | |
| path: e2e-tests/test-report-*.txt | |
| cargo test -p bb-flasher --tests --features ${{ matrix.test-suite.features }} -- --nocapture | |
| - name: Upload test report | |
| if: always() | |
| uses: actions/upload-artifact@v3 | |
| with: | |
| name: test-report-macos-${{ matrix.test-suite.features }} | |
| path: test-report-*.txt |
| name: E2E Tests | ||
|
|
||
| on: | ||
| pull_request: | ||
| branches: [ "main" ] | ||
| paths: | ||
| - 'bb-flasher/**' | ||
| - 'bb-flasher-sd/**' | ||
| - 'bb-flasher-bcf/**' | ||
| - 'bb-flasher-dfu/**' | ||
| - 'bb-flasher-pb2-mspm0/**' | ||
| - 'e2e-tests/**' | ||
| - '.github/workflows/e2e.yml' |
There was a problem hiding this comment.
The PR description focuses on enabling Tokio macros for bb-flasher-sd, but this change set also adds new E2E workflows/tests and IDE project files. Consider splitting these into separate PRs so the Tokio/doctest fix can be reviewed and landed independently from CI/testing infrastructure changes.
| //! E2E-style tests for SD card flashing. | ||
| //! | ||
| //! These tests exercise the SD flasher using a virtual file-backed device so | ||
| //! they can run in CI without requiring real SD card hardware. | ||
|
|
||
| use crate::e2e::{cleanup_test_file, create_test_image, create_virtual_sd_card}; | ||
|
|
||
| /// Basic SD card flashing with an uncompressed image into a virtual device. | ||
| #[tokio::test] | ||
| async fn sd_flash_uncompressed_to_virtual_device() { | ||
| const IMAGE_SIZE: usize = 4 * 1024 * 1024; // 4 MB | ||
| const SD_SIZE: usize = 16 * 1024 * 1024; // 16 MB | ||
|
|
||
| let img_path = create_test_image(IMAGE_SIZE).expect("failed to create test image"); | ||
| 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.
This test file is under the workspace root /tests, but the root Cargo.toml is a virtual manifest, so cargo test will not compile/run it. Additionally, it references crate::e2e::{... create_virtual_sd_card} which does not exist in the repo, and bb_flasher::sd::Target only accepts discovered removable devices (so a file-backed path will be rejected). Move these tests into a real crate (e.g. bb-flasher/tests/) or add an e2e-tests crate to the workspace and implement the missing helpers / a dedicated test backend.
| tempfile = "3" | ||
|
|
There was a problem hiding this comment.
tempfile is added as a dev-dependency but is not used by the integration tests in bb-flasher/tests/. Either remove it to keep the dependency surface minimal, or update the test helpers to use tempfile (e.g., NamedTempFile) instead of manually managing paths under temp_dir().
| tempfile = "3" |
| name: E2E Tests | ||
|
|
||
| on: | ||
| pull_request: | ||
| branches: [ "main" ] |
There was a problem hiding this comment.
There are now two separate workflows both named "E2E Tests" (e2e.yml and e2e-tests.yml) with overlapping triggers. This will duplicate CI runs and make results harder to interpret. Consider consolidating into a single workflow (or renaming/scoping triggers) so only one runs for a given change set.
Enable Tokio macros in bb-flasher-sd so the crate-level doctest using #[tokio::main] compiles on stable