Skip to content

test(fsim-upload): implement reliable FSIM upload E2E - #66

Merged
mmartinv merged 1 commit into
fido-device-onboard:mainfrom
say-paul:upload-test
Sep 24, 2025
Merged

test(fsim-upload): implement reliable FSIM upload E2E#66
mmartinv merged 1 commit into
fido-device-onboard:mainfrom
say-paul:upload-test

Conversation

@say-paul

@say-paul say-paul commented Sep 18, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor
  • Add test/e2e-fsim-upload.sh to exercise DI → RV → Owner (TO0/TO2) and verify FSIM upload end-to-end.
  • Refactor run_fido_device_onboard in test/test-makefile.sh to accept an “upload” mode: create uploaded.bin in the client working dir and run the client with --upload "/" so client reliably finds the file.
  • Verify upload by grepping Owner logs and confirming /tmp/go-fdo/uploads/uploaded.bin exists; verify checksum.

@say-paul
say-paul force-pushed the upload-test branch 2 times, most recently from 259b4e6 to 93c3f40 Compare September 18, 2025 10:02
mmartinv
mmartinv reviewed Sep 18, 2025
View reviewed changes

@mmartinv mmartinv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The idea is to use the existing functions and overwrite the ones that need changes, see test-containers.sh vs test-makefile.sh

If we want to run this test with both local run and containers you will need two files:

  • test-fsim-upload.sh (please let's try to be consistent with existing names) sourcing test-makefile.sh and overwriting/adding the required functions
  • test-fsim-upload-containers sourcing test-containers.sh and again overwriting/adding the required functions

Comment thread .github/workflows/e2e-fsim-upload.yml Outdated
Comment thread .github/workflows/e2e-fsim-upload.yml Outdated
Comment thread .github/workflows/e2e-fsim-upload.yml Outdated
Comment thread .github/workflows/e2e-fsim-upload.yml Outdated
@sarmahaj

sarmahaj commented Sep 18, 2025

Copy link
Copy Markdown
Collaborator

I agree with what @mmartinv has mentioned.
I think we can use the existing workflow.
So now we have two approaches 1.makefile based(runs fdo binaries) 2.Container based and 3.rpms (need to add test jobs there)
So for each environment we just over-ride the functions eg run_services() is when used in container env is different than when ran in makefile env. If you source test-container.sh, you get the container versions. If you source test-makefile.sh directly, you get the makefile versions.

So for download fsim I am trying to add a function test_download_fsim() in test-makefile.sh which will run for both the environments based on what is selected in ci.yml
we need to add some logic to detect which env it is running and then accordingly run required services. does that make sense ? @mmartinv

kgiusti
kgiusti reviewed Sep 18, 2025
View reviewed changes
Comment thread test/test-makefile.sh Outdated
mmartinv
mmartinv reviewed Sep 22, 2025
View reviewed changes

@mmartinv mmartinv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks great, just some minor function renaming to match the changes I am planning

Comment thread test/test-makefile.sh Outdated
Comment thread test/test-makefile.sh Outdated
Comment thread .github/workflows/ci.yml Outdated
Comment thread test/test-fsim-upload.sh Outdated
Comment thread test/test-fsim-upload.sh Outdated
Comment thread test/test-fsim-upload.sh Outdated
@mmartinv mmartinv mentioned this pull request Sep 22, 2025
Merged
@say-paul say-paul changed the title feat(test): implement reliable FSIM upload E2E test(fsim-upload): implement reliable FSIM upload E2E Sep 23, 2025
@say-paul

say-paul commented Sep 23, 2025

Copy link
Copy Markdown
Contributor Author

@mmartinv I committed all you suggested changes... and squashed them into one commit.

mmartinv
mmartinv reviewed Sep 23, 2025
View reviewed changes
Comment thread .github/workflows/ci.yml Outdated
Comment thread test/test-fsim-upload.sh Outdated
Comment thread test/test-fsim-upload.sh Outdated
Comment thread test/test-fsim-upload.sh Outdated
@say-paul
say-paul force-pushed the upload-test branch 2 times, most recently from 81ea802 to 5a2555d Compare September 23, 2025 15:29
@say-paul

say-paul commented Sep 23, 2025

Copy link
Copy Markdown
Contributor Author

@mmartinv addressed your comments, cleaned up other redundancies.

mmartinv
mmartinv reviewed Sep 23, 2025
View reviewed changes

@mmartinv mmartinv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's do the suggested minor changes and get this in.
You will also need to rebase and remove the changes in test/test-makefile.sh as they were already done by Ken in his PR.

Comment thread .github/workflows/ci.yml Outdated
Comment thread .github/workflows/ci.yml Outdated
- name: Cleanup the environment
if: always()
run: |
source test/test-makefile.sh

@mmartinv mmartinv Sep 23, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
source test/test-makefile.sh
source test/test-fsim-upload.sh

@say-paul @mmartinv
test(fsim-upload): validate onboarding process with FSIM upload enabled
37600c0 
Owner is started with --upload-directory and --command-upload.
Fdo device onboard is started with upload flag that points to the dir.
Matches source and destination file checksum.

Signed-off-by: Sayan Paul <paul.sayan@gmail.com>

Co-authored-by: Miguel Martin <mmartinv@redhat.com>
mmartinv
mmartinv approved these changes Sep 24, 2025
View reviewed changes

@mmartinv mmartinv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@mmartinv
mmartinv merged commit e342f41 into fido-device-onboard:main Sep 24, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kgiusti kgiusti kgiusti left review comments
@mmartinv mmartinv mmartinv approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@say-paul @sarmahaj @kgiusti @mmartinv