-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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
testutil: make testing packages public #39829
Conversation
Thanks @SamWhited! For posterity I like to put in the commit message at least, the exact sed command used. |
Sure. It was something along the lines of:
That may not be exact, I copied a few lines out of my bash history. I'll throw this in the commit message after CI finishes (I don't want to interrupt it) |
Two things still failing in CI, one appears to be the Windows issue that I believe is being worked on in another PR, the other doesn't appear to be related but I'm not sure why it's failing now. Will dig in as soon as I'm done with some of the buildkit changes. |
Looks like running goimports makes a lot of changes (including breaking ones because it gets confused by module version suffixes), fixing only the ones that were the result of the package rename to try and make things work for now. |
Remaining test failures are the windows failures which I think are failing in general right now and aren't related. PTAL. Thanks! |
Running tests on Windows appears to be failing for different reasons now, looking into it to see if it's my fault or not. They do appear to be failing (sometimes?) on master too, so maybe it's just flaky. If anyone is familiar with windows tests and what the state of testing on master is right now your input would be appreciated. Thanks! /cc @tiborvass, @thaJeztah EDIT: Ran it again and it's not a timeout anymore, it's EC2 failing so I'm going to go out on a limb and say this is just flaky right now. |
Rebased on top of Tibor's testing change. Waiting on CI to see if I missed anything or broken an import. |
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.
LGTM
Pushed a change to add import lines back as requested by @thaJeztah; PTAL, thanks! |
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.
LGTM
let's create a tracking issue to validate that each package has a (correct) import comment
@thaJeztah done, see #39898 |
Oh, sorry; this needs a rebase now; I see #39886 was merged, which caused a conflict |
Only failure was on Windows RS1, and a known flaky test (on slow platforms?) #38521;
|
@thaJeztah rebased |
This was done with something along the lines of: ``` mv internal/test testutil pushd testutil/; grep -IRl "package test" | xargs -I '{}' sed -i -e 's|package test|package testutil|g' {}; popd mv internal/testutil/*.go testutil/ && rm -rf internal/ grep -IRl "github.com\/docker\/docker\/internal\/test" | xargs -I '{}' sed -i -e 's|github.com/docker/docker/internal/test|github.com/docker/docker/test|g' {} goimports . ``` I also modified the basic plugin path in testutil/fixtures/plugin. Signed-off-by: Sam Whited <sam@samwhited.com>
Windows RS1 failure is a Jenkins issue; https://ci.docker.com/public/job/moby/job/PR-39829/17/execution/node/72/log/
Gonna ignore that one |
- What I did
Moved
/internal/test
and/internal/testutil
into/testutil
so that it can be used from within buildkit.- How I did it
Moved it and sed-ed all of the import paths and package names.
- How to verify it
Hopefully CI should pass and git should show that most files are just moved and not modified heavily.
- Description for the changelog
The testutil package is now public and can be used to spawn docker daemons for testing.
- A picture of a cute animal (not mandatory but encouraged)