Add E2E tests via GitHub actions #57
Conversation
ArangoGutierrez
requested review from
elezar,
tariq1890 and
cdesiniotis
as code owners
internal/logger/logger.go
Outdated
| @@ -130,7 +132,7 @@ func (l *FunLogger) Loading(format string, a ...any) { | |||
| defer l.Wg.Done() | |||
| message := fmt.Sprintf(format, a...) | |||
| // if running in a non-interactive terminal, don't print the loading animation | |||
| if !isInteractiveTerminal() && isCILogs() { | |||
| if !isInteractiveTerminal() && l.isCILogs() { | |||
There was a problem hiding this comment.
Question: Is isCILogs ever used alone? Does it make sense to be queried in isInteractiveTerminal() instead?
There was a problem hiding this comment.
no, is just used for this case, you are right
tests/Makefile
Outdated
|
|
||
| .PHONY: e2e-test | ||
| e2e-test: | ||
| @if [ -z ${AWS_SECRET_ACCESS_KEY} ] || [ -z ${AWS_ACCESS_KEY_ID} ]; then \ |
There was a problem hiding this comment.
Question: Doesn't holodeck already report an error if these are not set?
There was a problem hiding this comment.
the aws-sdk pkg will complain about it, but we don't have a proper check before calling the aws-sdk pkg's.
There was a problem hiding this comment.
My suggestion would be to remove this here (for simplicity) and create an issue to check these in code so that that can be reused in all consumers (device plugin, gpu operator, container toolkit) etc.
tests/holodeck_test.go
Outdated
| AfterAll(func(ctx context.Context) { | ||
| // Delete the environment |
There was a problem hiding this comment.
Should deletion be checked in the test too? I would say that holodeck fails if it doesn't delete an environment when requested.
There was a problem hiding this comment.
By doing the deletion here, we ensure that In the case of the create test fails, ginlgo/gomega will still trigger this function, vs if we set it as a "step" after the creation bit, if create fails, the delete part won't be called because the Expect(err).ToNot(HaveOccurred()) will exit the ginkgo container
There was a problem hiding this comment.
Let me put my question in another way: Assuming that create succeeds, will this raise an error if the environment is not deleted?
tests/holodeck_test.go
Outdated
| }) | ||
|
|
||
| Context("and calling dryrun to validate the file", func() { | ||
| It("validate the provider", func() { |
tests/holodeck_test.go
Outdated
| err = client.Delete() | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| }) | ||
| It("create the environment", func() { |
There was a problem hiding this comment.
"creates"
(Here I'm assuming that this is read as: "it creates the environment")
tests/holodeck_test.go
Outdated
| log := logger.NewLogger() | ||
| log.CI = true |
There was a problem hiding this comment.
Why not have functional options here instead?
tests/holodeck_test.go
Outdated
| Expect(err).ToNot(HaveOccurred()) | ||
|
|
||
| // set cache path | ||
| if opts.cachePath == "" { |
tests/holodeck_test.go
Outdated
| } | ||
| opts.cachefile = filepath.Join(opts.cachePath, opts.cfg.Name+".yaml") | ||
|
|
||
| opts.cfg.Spec.Provider = v1alpha1.ProviderAWS |
There was a problem hiding this comment.
This should probably be moved to after we read opts.cfg. Also, why are we overriding the provider -- should it not be defined in the spec or is the intent to use the same spec for multiple providers?
Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
ArangoGutierrez
force-pushed
the
e2e
branch
3 times, most recently
from
bc936e0 to
422d0b6
Compare
Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
No description provided.