chore: remove testify from cron e2e test suite #15138 #15214
Conversation
gianarb
force-pushed
the
chore/suite-without-testify
branch
from
0bad8d2 to
7fc7127
Compare
|
I do not like the name |
| if err != nil { | ||
| s.T().Fatal(err) | ||
| } | ||
| CheckError(s.T(), err) |
There was a problem hiding this comment.
Why both this and the previous changes of s.CheckError->CheckError.
This still should be an s.T().Helper()
test/e2e/fixtures/e2e_suite.go
Outdated
| var err error | ||
| s.RestConfig, err = kubeconfig.DefaultRestConfig() | ||
| s.CheckError(err) | ||
| CheckError(s.T(), err) |
There was a problem hiding this comment.
Can't we stick with s.CheckError(err) here and all the way down?
test/e2e/examples_test.go
Outdated
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| //kubeClient, err := kubernetes.NewForConfig(restConfig) |
There was a problem hiding this comment.
Remove dead code rather than comment it out please
| assert.Greater(t, cronWf.Status.LastScheduledTime.Time, time.Now().Add(-1*time.Minute)) | ||
| }) | ||
| } | ||
| func TestBasicTimezone(t *testing.T) { |
There was a problem hiding this comment.
Blank new line between functions please
test/e2e/cron_test.go
Outdated
| CronWorkflow(`apiVersion: argoproj.io/v1alpha1 | ||
| func TestBasic(t *testing.T) { | ||
| runner := fixtures.NewRunner(t) | ||
| defer runner.DeleteResources(t) |
There was a problem hiding this comment.
Requiring this everywhere is a bit messy and prone to mistakes (missing the call to DeleteResources). Also, it makes it impossible to t.Parallel() these as they all nuke all resources after they complete.
Can we run this in some wrapper like the former suite?
There was a problem hiding this comment.
The current cronjob suite does not support parallelization. defer does not look like the problem here. Parallelization will not work because the problem is in how tbe DeleteResources function works. It is a copy of the old one, and it does not support parallelization because there is no segmentation by test name. We should do something like labeling all the resources created in a test with their test name and the DeleteResources should delete only the one with such label. Right now the DeleteResources deletes across tests if I understand it correctly, and it works only because parallelization is disabled.
This is based on what we had before where the cleanup happened after each cron subtest. If we want to move it at the end of the all suite we can use the func TestMain(m *testing.M) structure but ready the comment related to why we do cleanup after each subtest I think we should keep it like it is so it means that in order to parallelize w eneed to do some work to improve the DeleteResources function
There was a problem hiding this comment.
Yeah, the current version doesn't support parallelisation, but it did used to have this enabled (but not working, due to testify). The linter testifylint complained about them having it enabled but disfunctional, so the code was removed.
Ok, let's go with this for now and we can parallelise in a separate PR if we ever get around to it.
I'd still like us not to have to remember the defer though, so wrapping it.
There was a problem hiding this comment.
Not sure if you like the approach, but I added options to the Runner and the cleanup runs if enabled as t.Cleanup function. It sounds idiomatic and I like the API. Let me know
This commit explores how to drop testify from another (first was example suite) e2e suite. In this case the cronjob one with particular focus on how to abstract the code used for both migrations in a reusable fashion Signed-off-by: Gianluca Arbezzano <ciao@gianarb.it>
gianarb
force-pushed
the
chore/suite-without-testify
branch
2 times, most recently
from
c9d6c50 to
4befc19
Compare
Signed-off-by: Gianluca Arbezzano <ciao@gianarb.it>
gianarb
force-pushed
the
chore/suite-without-testify
branch
from
4befc19 to
82fde00
Compare
2 participants
Fixes #15138
Motivation
This commit explores how to drop testify from another (first was example
suite) e2e suite. In this case the cronjob one with particular focus on
how to abstract the code used for both migrations in a reusable fashion
Modifications
I created a new structure to pre-configure and dispatch the Given fixture.
I refactored both Example and Cron suite to use it
Verification
CI/CD is green
Documentation
N/A