Move test-unit out of hack/make by dnephin · Pull Request #33987 · moby/moby

dnephin · 2017-07-06T16:41:23Z

This is similar to the changes we made to hack/validate. Running tests shouldn't require hack/make.sh. Moving them to hack/test/ will allow us to run them directly with less overhead. It also makes the scripts much easier to read because there's no hidden state provided by a "runner" (hack/make.sh).

Also:

Fixed a couple problems I found in Add test coverage to pkg/term #33520 (tests leaving the terminal broken, and remove the test flag)
Moved some functions out of hack/make.sh to where they are used. The less global state provided by hack/make.sh the easier these scripts are to read.
remove HAVE_GO_TEST_COVER which was dead code.

I'll address test-integration and test-docker-py as part of carrying #33344

thaJeztah · 2017-07-08T04:59:20Z

hack/make.sh

Looks like this is using spaces; can you use tabs for indenting?

Oops, it was using spaces before, I think I tried to convert it to a tab, but I ended will with just 4 spaces. Will fix

thaJeztah · 2017-07-08T05:01:54Z

Let's ping our resident bashologists

@tianon @albers PTAL

albers · 2017-07-11T17:15:08Z

hack/make/test-unit

I think it's confusing to have the redirection in front of the command. In fact I did not even know that this is possible. I'd prefer to have it at the end of the command.

Sounds good, moved to the end

albers · 2017-07-11T17:44:35Z

I'm not familiar with the build and test options, so almost every invocation variant I tried ended up in error messages. That's probably because I'm not familiar with go yet.
I'm also confused about the relationship between the makefiles, hack/make.sh and the files under hack/. There are hidden files, like .binary. Strange.
Is there a current documentation of how the scripts should be used?

The script changes all look reasonable to me, Most of it was shifted unchanged, so there is no point in commenting these parts. Only found a small nit, but this is not important. So LGTM.

dnephin · 2017-07-11T18:02:54Z

Re docs, hack/README.sh has some information which I think was added recently.

The relationship is something like this:

Makefile - builds images and runs containers
hack/make.sh - is a driver for most tasks. It sets some variables and sets a bundle/ path for any artifacts created by the task. Generally should be run from inside a container created by make shell or will be invoked by some other make target.
hack/make/* - scripts which are intended to be run by the hack/make.sh driver
hack/* - other scripts that aren't run by hack/make.sh

albers · 2017-07-12T11:46:05Z

While being at it, could you please remove from the ./hack/make.sh test example from hack/make/README.md?

albers · 2017-07-12T11:56:14Z

@dnephin thanks a lot for your hints. Now I could successfully execute the changed/new scripts.
I suppose the hidden files in hack/make are hidden because they do not correspond with make targets.

dnephin · 2017-07-12T14:11:23Z

Yes, I think a lot of the hidden files are meant to be like libraries that can be sourced, but do run directly.

dnephin · 2017-07-12T16:01:53Z

Removed the test example from the README

tianon

Seems sane to me!

tianon · 2017-07-14T22:02:10Z

hack/test/unit

Missed one 😄 (spaces vs tabs)

Thanks! fixed

Signed-off-by: Daniel Nephin <dnephin@docker.com>

Also remove the test flag from pkg/term and jsut checkuid directly. Fixed a problem with a pkg/term test that was leaving the terminal in a bad state. Signed-off-by: Daniel Nephin <dnephin@docker.com>

dnephin · 2017-07-26T19:32:33Z

ping for another LGTM

thaJeztah · 2017-07-26T20:47:20Z

pkg/term/term_linux_test.go

-var rootEnabled bool
-
-func init() {
-	flag.BoolVar(&rootEnabled, "test.root", false, "enable tests that require root")


do we know why this was here?

The intent was to only run some tests if the flag was set. I think the correct behaviour is to optimistically run the tests that can be run when the user is correct.

Could it be related to running the tests bare-metal? I seem to recall there were some tests in the past that could screw up the host (we had a fun one changing system time to verify certificate expiry)

ping @tiborvass @andrewhsu could you have a quick look to verify we can remove this?

These tests work with the terminal, so any failure to cleanup should be isolated to the terminal. This patch also includes a fix for a test that did not cleanup properly (defer RestoreTerminal()).

@thaJeztah @dnephin I think it's fine like this. We could do what dnephin said in https://github.com/moby/moby/pull/33520/files#r125954995 but honestly if you don't run it as root, you'll see the tests as skipped with a reason. The person reading the test logs should pay attention to skipped tests and take the decision of whether that is acceptable or not. That's how they would discover this behavior.

Thanks for looking; wanted to be sure I didn't miss anything 👍

tiborvass · 2017-07-26T22:28:02Z

LGTM

thaJeztah

LGTM

thaJeztah · 2018-07-03T14:30:53Z

hack/test/unit

+set -eu -o pipefail
+
+TESTFLAGS+=" -test.timeout=${TIMEOUT:-5m}"
+BUILDFLAGS=( -tags "netgo seccomp libdm_no_deferred_remove" )


Looks like I missed this; this changes the unit-tests to no longer use the BUILDFLAGS that were passed as env-var (and also changes them to be dynamically linked (previously statically))

Not using BUILDFLAGS was the intent. Those are the flags for building a binary, not necessarily running tests.

Does that cause a problem?

Caused me some digging why a backport didn't work, but worked on master 😂

Perhaps we should rename the variable here, to prevent it from being confused with the BUILDFLAGS in the Makefile and so on 🤔

GordonTheTurtle added the status/0-triage label Jul 6, 2017

dnephin added status/2-code-review and removed status/0-triage labels Jul 6, 2017

vdemeester requested review from tianon, tiborvass and vdemeester July 6, 2017 17:58

dnephin added the rebuild/windowsRS1 label Jul 6, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label Jul 6, 2017

dnephin added the rebuild/windowsRS1 label Jul 6, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label Jul 6, 2017

thaJeztah reviewed Jul 8, 2017

View reviewed changes

dnephin force-pushed the cleanup-more-hack branch from ccaf862 to e4acf9a Compare July 11, 2017 16:01

albers reviewed Jul 11, 2017

View reviewed changes

albers mentioned this pull request Jul 11, 2017

Introduce test-integration target #34000

Merged

dnephin force-pushed the cleanup-more-hack branch from e4acf9a to 6bfd986 Compare July 11, 2017 17:59

dnephin force-pushed the cleanup-more-hack branch from 6bfd986 to c356e6c Compare July 12, 2017 16:01

dnephin mentioned this pull request Jul 12, 2017

Add Loopback_test File #33368

Closed

dnephin added the rebuild/powerpc label Jul 12, 2017

GordonTheTurtle removed the rebuild/powerpc label Jul 12, 2017

tianon approved these changes Jul 14, 2017

View reviewed changes

dnephin added 2 commits July 17, 2017 11:38

More helper hack helper functions to a more appropriate place.

ece4520

Signed-off-by: Daniel Nephin <dnephin@docker.com>

Remove test-unit from hack/make

1fb6155

Also remove the test flag from pkg/term and jsut checkuid directly. Fixed a problem with a pkg/term test that was leaving the terminal in a bad state. Signed-off-by: Daniel Nephin <dnephin@docker.com>

dnephin force-pushed the cleanup-more-hack branch from c356e6c to 1fb6155 Compare July 17, 2017 15:39

dnephin added rebuild/powerpc labels Jul 17, 2017

GordonTheTurtle removed rebuild/powerpc labels Jul 17, 2017

thaJeztah added the area/testing label Jul 19, 2017

GordonTheTurtle assigned LK4D4 Jul 21, 2017

thaJeztah reviewed Jul 26, 2017

View reviewed changes

thaJeztah approved these changes Jul 26, 2017

View reviewed changes

thaJeztah merged commit f8c4343 into moby:master Jul 26, 2017

dnephin deleted the cleanup-more-hack branch July 26, 2017 22:43

thaJeztah mentioned this pull request Aug 16, 2017

Fix make test-unit printing deprecation warning #34536

Merged

dnephin mentioned this pull request Sep 20, 2017

Add a new entrypoint for CI #34911

Merged

thaJeztah reviewed Jul 3, 2018

View reviewed changes

Conversation

dnephin commented Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jul 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albers commented Jul 11, 2017

Uh oh!

dnephin commented Jul 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

albers commented Jul 12, 2017

Uh oh!

albers commented Jul 12, 2017

Uh oh!

dnephin commented Jul 12, 2017

Uh oh!

dnephin commented Jul 12, 2017

Uh oh!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephin commented Jul 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiborvass commented Jul 26, 2017

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dnephin commented Jul 6, 2017 •

edited

Loading

dnephin commented Jul 11, 2017 •

edited

Loading