test(e2e): add CLI test for pkg update #405
Conversation
antoineco
added
kind/enhancement
There was a problem hiding this comment.
All good here. Thanks!
Took a deep look and it seems fine + it seems that the test passed
Reviewed-by: Cezar Craciunoiu [email protected]
test/e2e/cli/pkg_test.go
Outdated
| Expect(manifestsPath).To(ContainFiles("index.yaml", "unikraft.yaml")) | ||
| Expect(manifestsPath).To(ContainDirectories("libs")) |
There was a problem hiding this comment.
These checks are manifest-specific artifacts. Whilst the default functionality of kraftkit is to use manifest Package Manager, it can be configured to use a different ones which would result in alternative output artefacts from kraft pkg update. For example, after the introduction of OCI package manager, sourcing from unikraft.org does not actually produce any system artefacts.
Perhaps just for greater clarity of the test(s) itself the package manager should be indicated? The flag -m should then be used to specifically set the manifest manager and the test name also includes this.
I can then see additional tests that try out different providers, e.g., starting from a blank list of unikraft manifests:
# e2e test 1: GitHub provider
kraft pkg source https://github.com/unikraft/unikraft.git
kraft pkg update
# only a `unikraft.yaml` artefact
# e2e test 2: GitProvider
[...]
# e2e test N: ManifestIndexProvider
kraft pkg source https://manifests.kraftkit.sh/index.yaml
# `index.yaml` and any additional "symlinks"
[...]
The current functionality assumes (afaik) that the config will be populated with its default configuration and values, meaning that the manifest provider is an ManifestIndexProvider set to the default location.
There was a problem hiding this comment.
How about having both? Test with a default config, and test with selected manifests?
We want to catch any breaking user facing change, and a change of default is exactly that, that's why I prioritized the default case here.
I'll be busy with other topics for the next few weeks most likely, so we can either block this PR until all cases are added, or benefit from these initial tests and iterate on additional cases later.
There was a problem hiding this comment.
I just nested the existing test cases inside a context block (see eedbbb1f..aa9bfc3b):
Context("implicitly using the default manager type (manifest)", { /* ... */ })This should make it explicit that we are testing with plain defaults, and that future context blocks will contain manager-specific test cases, alongside the default case.
PTAL
There was a problem hiding this comment.
Your suggestion of updating the test context helps assure that this test is intended for "default" functionality and addresses my concern. Thanks a tonne!
I will open up a separate issue addressing the permutations that can be made by the --manager flag which is out-of-scope for this initial e2e of kraft pkg update.
test/e2e/cli/pkg_test.go
Outdated
| // The help subsystem is managed by cobra and fails when top-level flags | ||
| // are passed, so we ensure to keep only the command name and subcommand | ||
| // from the original cmd. | ||
| cmd.Args = []string{cmd.Args[0], cmd.Args[len(cmd.Args)-2], cmd.Args[len(cmd.Args)-1], "--help"} |
There was a problem hiding this comment.
Non-blocking but this seems quite convoluted approach of injecting the --help flag (though the comment preamble does help) and will not scale if the parent BeforeEach includes any additional positional arguments or flags.
There was a problem hiding this comment.
Fully agree, this is based on the assumption that we are using a subcommand that is n-deep exactly. Mostly a workaround, the fact that we even need to avoid passing any other flag is a bug. I'll open an issue.
There was a problem hiding this comment.
I removed that hack entirely from here and it works.
The --config-dir flag doesn't cause any issue with kraft pkg update --help, but it still does with kraft version --help 🤔
Created #430
Signed-off-by: Antoine Cotten <[email protected]>
There was a problem hiding this comment.
Awesome! Looks good!
Reviewed-by: Alexander Jung [email protected]
Approved-by: Alexander Jung [email protected]
Prerequisite checklist
make fmton your commit series before opening this PR;Description of changes
Part of #370
Tests that, given an empty manifests directory,
kraft pkg updateretrieves the list of components, libraries and packages.Also tests that the command can print its own help.