Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add automated QEMU-based testing to CI passes (x86_64 only for now) #1041

Merged
merged 16 commits into from
Sep 19, 2023

Conversation

tsoutsman
Copy link
Member

@tsoutsman tsoutsman commented Sep 13, 2023

  • Added qemu_test application that automatically runs all tests and returns a specific exit code from QEMU based on output of tests.
  • Added a test Make target
  • Added a CI workflow that runs the test target
  • Changed test_mlx5, test_ixgbe, and test_block_io to return an exit code of 0 if the associated devices aren't connected. An alternative would be to add them to the skipped tests in qemu_test
  • Changed test_identity_mapping, test_aligned_page_allocation, and test_filerw to fail rather than print if they encounter an error.
  • Renamed tls_test to test_tls so it is detected by qemu_test.
  • Changed test_channel and test_tls to run all tests rather than specifying specific tests in the arguments.
  • Renamed test_serial_echo to serial_echo because it isn't really a test with defined success and failure conditions.
  • Changed test_task_cancel to always return 0 as task cancellation is not yet implemented.

I've also temporarily added test_channel to the list of skipped tests, because there seems to be a bug that causes deadlock. I'll need to look into the bug, but it's outside of the scope of this PR.

Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
@tsoutsman tsoutsman marked this pull request as draft September 13, 2023 00:24
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
2
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
3
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
@tsoutsman tsoutsman marked this pull request as ready for review September 13, 2023 01:19
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
@kevinaboos kevinaboos changed the title Add automated testing to CI Add automated QEMU-based testing to CI passes (x86_64 only for now) Sep 13, 2023
Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

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

Generally looks great, thanks so much! I left a few small comments.

One question -- how long does this take? I noticed a 10-min timeout in the action, hence my curiosity. Our existing CI passes already take over 5 min or so for basic PRs, so I'm wondering if there's a way to speed this up, e.g., by caching build results across separate action passes, and/or disabling actions when a PR's changeset doesn't touch files that require them to be re-run.

Makefile Outdated Show resolved Hide resolved
applications/test_channel/src/lib.rs Outdated Show resolved Hide resolved
kernel/first_application/src/lib.rs Outdated Show resolved Hide resolved
@tsoutsman
Copy link
Member Author

tsoutsman commented Sep 13, 2023

One question -- how long does this take?

The most recent commit took 7 min 27 sec.

EDIT: the most most recent commit took 5 min 26 sec, without any significant changes so 🤷 .

caching build results across separate action passes

It's possible, but I'm not sure how significant the speed up will be.

I don't think the time it takes to run CI is particularly important. CI speed is only relevant if a cleanup commit was pushed right before the PR is merged but Github has an auto-merge feature.

Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
@tsoutsman
Copy link
Member Author

Added the feature shenanigans in this commit. Not quite sure if that's the best way to do it.

@kevinaboos
Copy link
Member

kevinaboos commented Sep 14, 2023

CI speed is only relevant if a cleanup commit was pushed right before the PR is merged but Github has an auto-merge feature.

Well that's my primary use case, I do that for almost every PR so it would be nice to not make CI passes super slow. But nbd, i can wait 5-7 mins.

Added the feature shenanigans in this commit. Not quite sure if that's the best way to do it.

Ah i see, bitten yet again by the need for features to be additive. Yes, looking at that commit I do agree it's a bit weird, but we should be able to get around that by simply enabling first_application/qemu_test directly from the theseus_features crate. That way we wouldn't have to pass it through the captain for no reason.
Unfortunately, we won't be able to deactivate the shell dependency that way, as we can't use cfg(not(feature = "...")) in Cargo.toml manifests afaik, but you can do that in the Rust code itself. That would result in similar behavior to the cfg(qemu_test) approach, but with the added benefit of not including qemu_test in normal builds. You'll also have to remove the compile_error check, but basically the feature can be kept pseudo-additive by making qemu_test a preferential feature, in that it overrides the usage of shell in first_application whenever it is active.

Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
@tsoutsman
Copy link
Member Author

Should be good.

Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
@kevinaboos kevinaboos merged commit 7a8d64a into theseus-os:theseus_main Sep 19, 2023
3 checks passed
github-actions bot pushed a commit that referenced this pull request Sep 19, 2023
* Added `qemu_test` application that automatically runs all tests
  and returns a specific exit code from QEMU based on test output.

* Added a `test` Make target that enables and runs `qemu_test`.
  * Added a CI workflow that runs the `test` target

* Changed `test_mlx5`, `test_ixgbe`, and `test_block_io`
  to return an exit code of 0 if the required devices
  aren't connected.

* Changed `test_identity_mapping`, `test_aligned_page_allocation`,
  and `test_filerw` to fail rather than print if they encounter an error.

* Renamed `tls_test` to `test_tls` so it is detected by `qemu_test`.

* Changed `test_channel` and `test_tls` to run all tests
  rather than specifying specific tests in the arguments.

* Renamed `test_serial_echo` to `serial_echo` because it
   isn't really a test with defined success and failure conditions.

* Changed `test_task_cancel` to always return 0, because
  task cancellation is not yet implemented in the mainline.

* Skip `test_channel`, as it currently does not work.

Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com> 7a8d64a
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.

2 participants