-
Notifications
You must be signed in to change notification settings - Fork 597
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
New test tooling #3418
New test tooling #3418
Conversation
8cb9f25
to
814418a
Compare
37bc5e9
to
193a67d
Compare
@AkihiroSuda this is ready for a first review. CI is pending. |
WithConfig(key ConfigKey, value ConfigValue) Data | ||
ReadConfig(key ConfigKey) ConfigValue | ||
|
||
// Private methods |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private methods do not need to be defined in the interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do need getLabels and getConfig here:
nerdctl/pkg/testutil/test/data.go
Lines 131 to 137 in f1434f4
func configureData(t *testing.T, seedData Data, parent Data) Data { | |
if seedData == nil { | |
seedData = &data{} | |
} | |
dat := &data{ | |
config: seedData.getConfig(), | |
labels: seedData.getLabels(), |
Will remove adopt
, which is not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (removed adopt
).
Lmk if there is a better way to achieve this without defining the other two on the interface.
"github.com/containerd/nerdctl/v2/pkg/inspecttypes/dockercompat" | ||
"github.com/containerd/nerdctl/v2/pkg/inspecttypes/native" | ||
"github.com/containerd/nerdctl/v2/pkg/testutil" | ||
"github.com/containerd/nerdctl/v2/pkg/testutil/test" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why separate pkg?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are nerdctl specific (NerdctlToml, etcetera). Motivation was that there is already a lot inside the test
package, and the nerdtest
package will likely grow a lot more as it subsumes base
.
I can merge them back together though if preferable. No strong opinion here.
6b60abc
to
28263b1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
I forgot to convert some tests that are commented out. |
28263b1
to
4a27229
Compare
@AkihiroSuda last push finish converting and re-enabled the mistakenly commented out tests. Also note a small change in the test tooling, enabling |
CI failures are network flukes, which have been endemic with github recently. |
Is this ready to be merged, or do you plan to migrate more tests in this single PR? |
I think we should merge now if you are ok with it. I would like to use the new tooling in other PRs (like the "rewrite cp" one). I'll migrate more tests separately. This is going to take time. |
4a27229
to
5d9e243
Compare
Latest push has minor wording fixes in the documentation. |
Our testing tools have served us well, and there are plenty of good things with them that we absolutely need to keep. We have grown a large test suite though, and the sheer size of it is putting pressure on it and it is starting to show cracks. Besides bugs per-se (double execution) - that we hopefully fixed - our design is reaching its limits. We do have rather impactful issues with regard to test isolation and test concurrency, leading to situations where it is hard or outright impossible to figure out which test is causing a cascading failure, or why the adding of a new test file working individually breaks everything. Furthermore, as we are not prescriptive on certain things, we do see a lot of negative patterns emerging from test authors: - defer being used instead of t.Cleanup - not calling cleanup routines before running tests - outright forgetting to cleanup resources - Cwd for binary calls is by default the current working directory of the test process - this is causing a variety of issues, as it could very well be read-only (lima), and should by default be a temp directory - manipulating the environment directly - which has side-effects for other tests - tests becoming big blurbs of mixed together setup, cleanup, and actual test routines - making them hard to read and figuring out what is actually being tested - subtests repetitiveness w. shadowing testutil.T leading to confusing code - ... or not dereferencing the current test in a loop - in-test homegrown abstractions being inherently repetitive, with the same boilerplate over and over again - structuring tests and subtests being left as an exercise to the developer - leading to a wide variety of approaches and complex boilerplate - hard to debug: a lot of the assert methods do not provide any feedback whatsoever on what was the command that actually failed, or what was the output, or environment of it - icmd.Expected showing its limits, and making assumptions that there is only one error you can test, or that you can only test the output if exitCode == 0 - running commands with other binaries than the current target (eg: not calling base.Cmd) being left as an exercise to the developer, leading to all of the issues above (Chdir, Env manipulation, no debugging output, etc) - no-parallel being the default - unless specified otherwise - which should be the other way around - very rarely testing stderr in case of error - partly because we do not use typed errors, but also because it is cumbersome / not obvious / limited This new tooling offers a set of abstractions that should address all of these and encourage more expressive, better structured, better isolated, more debuggable tests. Signed-off-by: apostasie <[email protected]>
5d9e243
to
0eb301e
Compare
Effed-up my commit rewrite :/. Hang on a sec. |
39b55d7
to
ac18215
Compare
Signed-off-by: apostasie <[email protected]>
Signed-off-by: apostasie <[email protected]>
ac18215
to
a74f0f3
Compare
All set. Apologies for the back and forth. While I was in the process of rewriting more tests this weekend, a few fixes and changes came up as necessary for the tooling, which is better to have here directly:
Thanks for your patience... This task is a large and tricky endeavor, but we are starting to see benefits:
Merging at your convenience as other PRs would (mutually) benefit (from) the new testing tools. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
We are not in a good situation wrt our testing. The build would be constantly red if it wasn't for the fact we are retrying every test multiple times. Some of it may be bugs, but most of it is definitely broken tests.
Despite lengthy efforts to reduce flakiness one by one, it seems unlikely to me that we can make significant progress without better test tooling to use.
This here proposes a new testing approach that should hopefully address our endemic issues and promote better testing in the future, by providing by default better test isolation, simpler, more debuggable helpers, and overall a more expressive testing approaching.
Commit 1 is the tooling itself.
Commit 2 is a handful of tests being rewritten with the new tooling.
Commit 3 is documentation.
Background information:
Obviously, this should keep evolving a lot, and some stuff might have to change as we rewrite more tests and discover more use-cases.
I would suggest reading the
testtool.md
doc first before engaging with code.Right now, what matters most IMHO is that:
Improvements can always follow.