Revamp registry test #3076
Revamp registry test #3076
Conversation
apostasie
force-pushed
the
dev-3044
branch
4 times, most recently
from
117ac97 to
11c8504
Compare
| // skip if nydusify is not installed | ||
| // skip if nydusify and nydusd are not installed | ||
| testutil.RequireExecutable(t, "nydusify") | ||
| testutil.RequireExecutable(t, "nydusd") |
There was a problem hiding this comment.
Adding missing requirement
| defer registry.Cleanup() | ||
| registry := testregistry.NewWithNoAuth(base, 0, false) | ||
| remoteImage := fmt.Sprintf("%s:%d/nydusd-image:test", "localhost", registry.Port) | ||
| t.Cleanup(func() { |
There was a problem hiding this comment.
Cleanup is better to use than defer
apostasie
force-pushed
the
dev-3044
branch
2 times, most recently
from
0c22fba to
7c80d34
Compare
|
CI is green @AkihiroSuda and team PTAL at your convenience. |
Signed-off-by: apostasie <[email protected]>
|
Apologies for the re-push. |
|
@AkihiroSuda if this looks fine to you folks, I would love to see this merged in as I have more CI and tests changes that would benefit from it. I believe the windows failure is a fluke, so, if you can poke it that would be lovely. |
|
|
||
| reg := testregistry.NewPlainHTTP(base, 5000) | ||
| defer reg.Cleanup() | ||
| reg := testregistry.NewWithNoAuth(base, 0, false) |
There was a problem hiding this comment.
Methods have been simplified / renamed:
- there is no longer separate helpers for http vs. https (you pass a tls bool instead) - so, the previous name (NewPlainHTTP, that does not specific what type of auth it is using)
- all helpers names now follow the same pattern:
- NewWithTokenAuth
- NewWithBasicAuth
- NewWithNoAuth
Let me know if you don't like it and I can revert this one if you wish.
| t.Logf("localhost IP=%q", localhostIP) | ||
| testImageRefPrefix := fmt.Sprintf("%s:%d/", | ||
| localhostIP, reg.ListenPort) | ||
| localhostIP, reg.Port) |
There was a problem hiding this comment.
"ListenIP" makes sense (0.0.0.0) - as in "the address the registry process is binding to" - and also does "IP" (the exposed ip you can access it with).
"ListenPort" feels the wrong way - since this is NOT the port the registry process is actually listening on (still 5000), but instead the port you can access it with.
It seems better to me that way.
Either way, this is minor. I can reverse it if you dont like it.
Thanks @AkihiroSuda
2 participants
The fundamental motivation for this is to fix #3044
Furthermore, before embarking on revamping login code to fix everything in #3072 we do need a clean working ready-to-go test tooling and environment (specifically fix #3071).
This PR does revamp
testregistry,testcaand adds a couple of files.In a shell:
testcanow allows additional namesportlockallows for "locking" a port (or finding a free one), that is not racy and safe to use withparallelacross distinct invocations. This allows tests to not have to specify an explicit port anymore (if they don't want a specific one), which in turn allow for parralelization without risks of conflictstestregistryandlogin_linux_testhave been largely rewritten and fix:.docker/config.jsontestregistryPending green CI, I would suggest we merge this ASAP so that further test cleanup can happen (and more parallelization) and actual work can begin on cleaning-up login