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.

Sign up for GitHub

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

Jump to bottom

Setup integration tests #619

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Setup integration tests #619

wants to merge 5 commits into from

Conversation

fitzthum
Copy link
Member

Creates a framework for writing Trustee integration tests in Rust.

Put your tests in integration-tests/tests/integration.rs (or a similar file) and they will run when you do cargo test at the top level.

I've added two simple integration tests. They aren't anything beyond what we already have in other tests, but they add a structure that should help us to create a lot more. Before I get too carried away, please take a look at the setup.

Note that running the KBS in the background was trickier than expected. I think I found a pretty good way to do it in the end.

@fitzthum
token: make token module public
050b619 
Without this we can't create a token configuration from outside the
crate.

Signed-off-by: Tobin Feldman-Fitzthum <[email protected]>
@fitzthum fitzthum requested a review from a team as a code owner December 10, 2024 20:23
@Xynnn007
Copy link
Member

Looks great! One question before getting into the code:

why not we directly have an tests directory under kbs to have these integration tests, like what we do in image-rs

A quick glance is that way we need to add kbs_client to kbs's [dev-dependencies] section in Cargo.toml.

This way, we could avoid another sub crate.

@fitzthum
Copy link
Member Author

fitzthum commented Dec 11, 2024 •
edited
Loading

why not we directly have an tests directory under kbs to have these integration tests, like what we do in image-rs

I think this would work. I see the integration tests as having a broader scope than just the KBS, so I made a new package, but if that adds overhead I could move it. Generally I think that since the repo merge, we've had some stuff in individual packages that should really be in the top of the repo (like configs and docs, perhaps).

@fitzthum fitzthum force-pushed the setup-integration-tests branch from 1adf56a to b311ab0 Compare December 11, 2024 19:15
Xynnn007
Xynnn007 reviewed Dec 12, 2024
View reviewed changes
Copy link
Member

@Xynnn007 Xynnn007 left a comment

Choose a reason for hiding this comment

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

I think it would make sense to have a separate crate if we want to add more tests. One question is that we are having some e2e test

  • docker-compose ci
  • e2e test here
  • this PR

Do we need to delete some duplicated ones?

integration-tests/tests/integration.rs Outdated Show resolved Hide resolved
integration-tests/Cargo.toml Outdated Show resolved Hide resolved
integration-tests/tests/integration.rs
allow = false
";

#[allow(dead_code)]
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this lint macro?

Copy link
Member Author

@fitzthum fitzthum Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The Custom and DenyAll variants are never instantiated. I could move the dead code thing just to those variants but then I'd have to use it twice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we comment them out if they just have informational value?

integration-tests/tests/integration.rs Outdated Show resolved Hide resolved
@fitzthum fitzthum force-pushed the setup-integration-tests branch from b311ab0 to 5d3767a Compare December 12, 2024 19:55
@fitzthum
Copy link
Member Author

@Xynnn007

Do we need to delete some duplicated ones?

Since these integration tests use the crate directly, they don't test the binaries or the docker-compose configuration or the k8s configuration. I think it's fine to keep some simple tests around that make sure that those interfaces still work. We should do a little bit of cleanup and organization with our existing tests, but I think that's for another PR.

@fitzthum fitzthum force-pushed the setup-integration-tests branch 3 times, most recently from 669f8e6 to 75dee21 Compare December 20, 2024 22:45
@fitzthum
api_server: add option to start api server in background
c65676c 
The existing serve function returns a future that must be awaited on for
the server to run. This causes the caller to block and does not allow
the server to be halted unless the process is terminated.

This is not ideal when using the server as a crate (or in a test).

Instead, add a function that returns a server which can then be awaited
on or run in the background.

Signed-off-by: Tobin Feldman-Fitzthum <[email protected]>
@fitzthum
tests: add integration test framework
a1c6156 
To support integration tests written in rust, let's add a new package
(which will only be used internally) called integration-tests.

Inside the package, we have a tests repo where we can put tests.

The package imports other trustee packages using a local path,
so it should always have the most recent version of things.

Now integration tests will run simply by doing cargo test at the top of
the repo.

Signed-off-by: Tobin Feldman-Fitzthum <[email protected]>
@fitzthum
tests: add integration test to Makefile
8ffcff3 
Let the make check target (which maybe should be renamed?) also run the
integration tests.

Signed-off-by: Tobin Feldman-Fitzthum <[email protected]>
@fitzthum fitzthum force-pushed the setup-integration-tests branch 2 times, most recently from 89754d0 to fb4069f Compare December 20, 2024 23:12
@fitzthum
policy: fixup simple default policy
ce92336 
The OPA linter seems to have updated and now complains if we don't use
an if before our blocks.

This is unrelated to the rest of this PR.

Signed-off-by: Tobin Feldman-Fitzthum <[email protected]>
@fitzthum fitzthum force-pushed the setup-integration-tests branch from fb4069f to ce92336 Compare December 20, 2024 23:19
portersrc
portersrc reviewed Jan 8, 2025
View reviewed changes
attestation-service/src/token/simple_default_policy.rego
@@ -28,10 +28,11 @@
package policy

import future.keywords.every
import future.keywords.if
Copy link
Member

Choose a reason for hiding this comment

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

what's going on here? was if not in the original simple-default-policy and should have been (so you added it)?

mkulke
mkulke approved these changes Jan 8, 2025
View reviewed changes
integration-tests/tests/integration.rs
allow = false
";

#[allow(dead_code)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we comment them out if they just have informational value?

portersrc
portersrc reviewed Jan 8, 2025
View reviewed changes
kbs/Makefile
@@ -81,7 +81,7 @@ uninstall:
rm -rf $(INSTALL_DESTDIR)/kbs $(INSTALL_DESTDIR)/kbs-client $(INSTALL_DESTDIR)/issuer-kbs $(INSTALL_DESTDIR)/resource-kbs

check:
cargo test -p kbs -p kbs-client
cargo test -p kbs -p kbs-client -p integration-tests
Copy link
Member

Choose a reason for hiding this comment

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

This change will get hit here in our regular workflows, right? (If so, that's what we want, I think.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Xynnn007 Xynnn007 Xynnn007 left review comments

@portersrc portersrc portersrc left review comments

@mkulke mkulke mkulke approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Trustee Roadmap
Status: We have code
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fitzthum @Xynnn007 @mkulke @portersrc