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

Revamp e2e test suite #623

Merged
merged 2 commits into from Aug 10, 2023
Merged

Revamp e2e test suite #623

merged 2 commits into from Aug 10, 2023

Conversation

diconico07
Copy link
Contributor

What this PR does / why we need it:
Move to pytest based test-suite, and ensure tests can be run more easily locally.
The main reasons to move to pytest are:

  • Allows to run a subset of the tests rather than full suite
  • Ensure better separation of tests vs helpers
  • Easily share code for setup/teardown between suites

This commit also change dependency management to use poetry, this allows to setup the test environment locally in an easy and predictable way, it also ensure more reproducibility for our test-suite as the dependencies are pinned.

On a more functional end, switch to slim agent to allow test suites to chose their discovery handlers. Also stop using helm for configuration deployment to decorrelate it from akri's installation, preventing possible noise in tests.

Also makes use of watch instead of polling as much as possible to reduce waiting time (goes from about 3 minutes for e2e run to about 80 seconds).

Hopefully this will make it easier to write more e2e tests.

Special notes for your reviewer:

If applicable:

  • this PR has an associated PR with documentation in akri-docs
  • this PR contains unit tests
  • added code adheres to standard Rust formatting (cargo fmt)
  • code builds properly (cargo build)
  • code is free of common mistakes (cargo clippy)
  • all Akri tests succeed (cargo test)
  • inline documentation builds (cargo doc)
  • all commits pass the DCO bot check by being signed off -- see the failing DCO check for instructions on how to retroactively sign commits

@diconico07
Copy link
Contributor Author

Currently this is still WIP, the webhook suite is not migrated, the old suite is not deleted and this will also need a proper documentation PR, so I send this as a draft to get some comments about this, so please still take a look and tell me.

@diconico07 diconico07 force-pushed the revamp-e2e branch 6 times, most recently from 074e959 to 02a8ba9 Compare June 28, 2023 14:59
@diconico07
Copy link
Contributor Author

Only the documentation part is lacking before getting this out of draft

@diconico07
Copy link
Contributor Author

/add-same-version-label

@github-actions
Copy link
Contributor

👋 Added [same version] label :)!

@github-actions
Copy link
Contributor

👋 Added [same version] label :)!

@diconico07 diconico07 force-pushed the revamp-e2e branch 4 times, most recently from 0f2a072 to b4a6e47 Compare July 20, 2023 08:25
@diconico07 diconico07 mentioned this pull request Jul 25, 2023
8 tasks
Copy link
Contributor

@kate-goldenring kate-goldenring left a comment

Choose a reason for hiding this comment

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

Thank you for putting this together. This is a very clean setup. Just took a brief look -- will look back at it.

test/e2e/pyproject.toml Outdated Show resolved Hide resolved
.github/workflows/run-test-cases.yml Show resolved Hide resolved
def distribution_config(pytestconfig):
distribution = pytestconfig.getoption("--distribution", None)
if distribution == "k3s":
yield Distribution("k3s", Path.home() / ".kube/config", "kubectl")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we update the tests and workflow to match the distribution defaults to better enable running the tests locally without having to copy over the kubeconfig?

Suggested change
yield Distribution("k3s", Path.home() / ".kube/config", "kubectl")
yield Distribution("k3s", Path.home() / "/etc/rancher/k3s/k3s.yaml", "kubectl")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure of the added benefit for local runs, as I think most people will either have a long running instance they use for tests (so probably already configured) or use tools like k3d or rancher desktop that already manage the configuration.

In fact I first wanted to just get rid of that element here (and let the python kubernetes client handle it the same way the helm or kubectl commands do), but wasn't really sure it was the right thing to do.

Move to pytest based test-suite, and ensure tests can be run more easily
locally.
The main reasons to move to pytest are:
 - Allows to run a subset of the tests rather than full suite
 - Ensure better separation of tests vs helpers
 - Easily share code for setup/teardown between suites

This commit also change dependency management to use poetry, this allows
to setup the test environment locally in an easy and predictible way, it
also ensure more reproductability for our test-suite as the dependencies
are pinned.

On a more functional end, switch to slim agent to allow test suites to
chose their discovery handlers. Also stop using helm for configuration
deployment to decorelate it from akri's installation, preventing
possible noise in tests.

Also makes use of watch instead of polling as much as possible to reduce
waiting time (goes from about 3 minutes for e2e run to about 80
seconds).

Hopefully this will make it easier to write more e2e tests.

Signed-off-by: Nicolas Belouin <nicolas.belouin@suse.com>
Signed-off-by: Nicolas Belouin <nicolas.belouin@suse.com>
Copy link
Contributor

@kate-goldenring kate-goldenring left a comment

Choose a reason for hiding this comment

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

This is so nice. Thank you for putting this together, Nicolas!

@diconico07 diconico07 merged commit b6b86fa into project-akri:main Aug 10, 2023
16 checks passed
@diconico07 diconico07 deleted the revamp-e2e branch August 10, 2023 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

End to end tests are ridiculously complicated and maybe impossible to run locally
3 participants