mount: also test against local mountinfo source code #68

thaJeztah · 2021-03-09T10:49:52Z

mount depends on mountinfo, but defaults to the release specified in go.mod.

This change adds a new test to also run against the local code so that we can catch regressions / breaking changes.

thaJeztah · 2021-03-09T10:54:10Z

Makefile

+.PHONY: test-local
+test-local: RUN_VIA_SUDO = $(shell $(SUDO) true && echo -exec \"$(SUDO)\")
+test-local: clean
+	echo "replace github.com/moby/sys/mountinfo => ../mountinfo" | cat mount/go.mod - > mount/go-local.mod \
+	&& cd mount \
+	&& go test -modfile=go-local.mod $(RUN_VIA_SUDO) -v . ;\
+	rm go-local.*
+	cd ../


I must admit that I've been going back-and-forth what the cleanest way is to implement this;

use mktemp -d to create a temporary directory for the go-local.mod ?

note that bash "pseudo files" (<(echo "replace github.com/moby/sys/mountinfo => ../mountinfo" | cat mount/go.mod -)) don't work; go REQUIRES a file, and it must have a .mod extension (possibly something we should request to be changed, so that, e.g. STDIN / - could be supported?

combine this with the test: target? (less flexible, but perhaps cleaner?)

split creation of mount/go-local.md to a separate target?

Input/suggestions welcome; my initial goal here is to have some way to test against the local source 😅

thaJeztah · 2021-03-09T11:16:32Z

@kolyshkin ptal; happy to hear your thoughts on this

kolyshkin · 2021-03-13T02:50:50Z

Perhaps we can just add the replace line to go.mod, do the test and remove it. Add it with a comment to have a marker to aid in removal.

Something like this (as a script in mount directory, UNTESTED):

#!/bin/bash
MARKER='# Added temporarily by test-latest'

trap "sed -i \"/^${MARKER}/,$d\" go.mod" EXIT

if ! grep -q "^${MARKER}$" go.mod; then
        cat << EOF >> go.mod
$MARKER
replace replace github.com/moby/sys/mountinfo => ../mountinfo
EOF
fi

go test -v .

And in the top makefile:

.PHONY: test-latest
test-latest:
        ./mount/test-latest

Can also be done entirely in Makefile, of course...

thaJeztah · 2021-04-06T22:11:31Z

Perhaps we can just add the replace line to go.mod, do the test and remove it. Add it with a comment to have a marker to aid in removal.

I considered something like that, but (personally) prefer not to modify files that are in version control; that way we also don't have to revert the change (and allow running it locally as well, without accidentally modifying the file)

kolyshkin · 2021-04-06T22:24:26Z

.github/workflows/test.yml


      - name: tests
-        run: ssh default 'cd /vagrant && make test'
+        run: ssh default 'cd /vagrant && make test && make test-local'


can be just make test test-local

...but rather than having a need to call test-local separately, I'd add test: test-local dependency into the Makefile.

Yes, makes sense; tests run really fast, so shouldn't be an issue to always run both

kolyshkin · 2021-04-06T22:27:56Z

Makefile

+	echo 'replace github.com/moby/sys/mountinfo => ../mountinfo' | cat mount/go.mod - > mount/go-local.mod \
+	&& cd mount \
+	&& go mod download -modfile=go-local.mod \
+	&& go test -modfile=go-local.mod $(RUN_VIA_SUDO) -v . ;\


nit: add a space before \.

With your other comments about continuation; I'll remove this, and make the rm just rm -f mount/go-local.*

kolyshkin · 2021-04-06T22:29:57Z

Makefile

+.PHONY: test-local
+test-local: RUN_VIA_SUDO = $(shell $(SUDO) true && echo -exec \"$(SUDO)\")
+test-local: clean
+	echo 'replace github.com/moby/sys/mountinfo => ../mountinfo' | cat mount/go.mod - > mount/go-local.mod \


Here continuation (\ and && on the next line) is not needed. Make executes the commands via shell one by one, if a command fails, it stops.

The continuation is needed after cd as it won't affect the next command.

Ah! Good one; I sometimes get confused when to \ and when not in Makefiles

kolyshkin · 2021-04-06T22:30:20Z

Makefile

+	&& go mod download -modfile=go-local.mod \
+	&& go test -modfile=go-local.mod $(RUN_VIA_SUDO) -v . ;\
+	rm go-local.*
+	cd ../


This line is not needed (every command is run via its own shell).

kolyshkin · 2021-04-06T22:31:48Z

Makefile

 .PHONY: test
 test: RUN_VIA_SUDO = $(shell $(SUDO) true && echo -exec \"$(SUDO)\")
-test:
+test: clean


Not needed? Even if mount/go-local.mod is present, it does not affect go test in any way (AFAIK).

kolyshkin · 2021-04-06T22:33:05Z

Makefile

+# changes in mountinfo.
+.PHONY: test-local
+test-local: RUN_VIA_SUDO = $(shell $(SUDO) true && echo -exec \"$(SUDO)\")
+test-local: clean


clean dependency is not needed, as we're overwriting the file anyway.

mount depends on mountinfo, but defaults to the release specified in go.mod. This change adds a new test to also run against the local code so that we can catch regressions / breaking changes. Signed-off-by: Sebastiaan van Stijn <[email protected]>

kolyshkin · 2021-05-11T19:16:38Z

Makefile

+	$(RM) mount/go-local.*

 .PHONY: test
 test: RUN_VIA_SUDO = $(shell $(SUDO) true && echo -exec \"$(SUDO)\")


I would change this to

Suggested change

test: RUN_VIA_SUDO = $(shell $(SUDO) true && echo -exec \"$(SUDO)\")

test test-local: RUN_VIA_SUDO = $(shell $(SUDO) true && echo -exec \"$(SUDO)\")

(and move up) to avoid repetition.

(and move up) to avoid repetition.

You mean, move test-local above test ?

I mean, combine the two lines

test: RUN_VIA_SUDO = $(shell $(SUDO) true && echo -exec \"$(SUDO)\") .... test-local: RUN_VIA_SUDO = $(shell $(SUDO) true && echo -exec \"$(SUDO)\")

into one

test test-local: RUN_VIA_SUDO = $(shell $(SUDO) true && echo -exec \"$(SUDO)\")

and move it up next to SUDO definition.

kolyshkin · 2021-05-11T19:20:15Z

Makefile

+	cd mount \
+	&& go mod download -modfile=go-local.mod \
+	&& go test -modfile=go-local.mod $(RUN_VIA_SUDO) -v .


Suggested change

cd mount \

&& go mod download -modfile=go-local.mod \

&& go test -modfile=go-local.mod $(RUN_VIA_SUDO) -v .

cp mount/go.sum mount/go-local.sum

cd mount && go test -modfile=go-local.mod $(RUN_VIA_SUDO) -v .

or

Suggested change

cd mount \

&& go mod download -modfile=go-local.mod \

&& go test -modfile=go-local.mod $(RUN_VIA_SUDO) -v .

ln -sf go.sum mount/go-local.sum

cd mount && go test -modfile=go-local.mod $(RUN_VIA_SUDO) -v .

I'm not sure if understand this change; go-local.sum will be different than go.sum, because it's using different versions of the dependency; the go mod download -modfile=go-local.mod may pull other versions of dependencies, because mountinfo ("main branch") may be using different versions of dependencies.

Took me a while to figure out why I originally suggested that :)

This is because

the only difference between go.mod and go-local.mod is the replace directive we add;

go mod download is not needed at all -- there is nothing to download, the module is here already;

for the modules with relevant paths (aka local modules), no checksums are ever needed or recorded.

This means that existing go.sum file is totally adequate, so copying or linking it is sufficient. You can even try to do go mod tidy -modfile go-local.mod and see what happens -- in fact it will remove the checksums for mountinfo v0.4.1 package (as it is no longer used), but having this extra lines is not a problem for tests.

kolyshkin · 2021-09-20T19:44:42Z

@thaJeztah PTAL

kolyshkin · 2021-10-25T17:35:48Z

Makefile


 .PHONY: all
-all: lint test cross
+all: clean lint test test-local cross


Nit: no need to have test-local here as test depends on it.

kolyshkin · 2021-10-25T18:18:17Z

I have carried this to #87, with the following changes (explained above):

diff --git a/Makefile b/Makefile
index d7683d9..b4a196e 100644
--- a/Makefile
+++ b/Makefile
@@ -4,31 +4,29 @@ BINDIR ?= _build/bin
 CROSS ?= linux/arm linux/arm64 linux/ppc64le linux/s390x \
        freebsd/amd64 openbsd/amd64 darwin/amd64 darwin/arm64 windows/amd64
 SUDO ?= sudo -n
+test test-local: RUN_VIA_SUDO = $(shell $(SUDO) true && echo -exec \"$(SUDO)\")
 
 .PHONY: all
-all: clean lint test test-local cross
+all: clean lint test cross
 
 .PHONY: clean
 clean:
        $(RM) mount/go-local.*
 
 .PHONY: test
-test: RUN_VIA_SUDO = $(shell $(SUDO) true && echo -exec \"$(SUDO)\")
 test: test-local
        for p in $(PACKAGES); do \
                (cd $$p && go test $(RUN_VIA_SUDO) -v .); \
        done
 
-# test the mount module against the local mountinfo source code instead of the
-# release specified in go.mod. This allows catching regressions / breaking
+# Test the mount module against the local mountinfo source code instead of the
+# release specified in its go.mod. This allows catching regressions / breaking
 # changes in mountinfo.
 .PHONY: test-local
-test-local: RUN_VIA_SUDO = $(shell $(SUDO) true && echo -exec \"$(SUDO)\")
 test-local:
        echo 'replace github.com/moby/sys/mountinfo => ../mountinfo' | cat mount/go.mod - > mount/go-local.mod
-       cd mount \
-       && go mod download -modfile=go-local.mod \
-       && go test -modfile=go-local.mod $(RUN_VIA_SUDO) -v .
+       cp mount/go.sum mount/go-local.sum
+       cd mount && go test -modfile=go-local.mod $(RUN_VIA_SUDO) -v .
        $(RM) mount/go-local.*
 
 .PHONY: lint

thaJeztah commented Mar 9, 2021

View reviewed changes

thaJeztah force-pushed the test_tip_tip branch from cb83d78 to fcf8526 Compare March 9, 2021 11:03

kolyshkin reviewed Apr 6, 2021

View reviewed changes

thaJeztah force-pushed the test_tip_tip branch 3 times, most recently from dcd4a3f to 12e05c2 Compare April 7, 2021 16:48

thaJeztah force-pushed the test_tip_tip branch from 12e05c2 to 13d0624 Compare April 7, 2021 16:56

kolyshkin reviewed May 11, 2021

View reviewed changes

kolyshkin reviewed Oct 25, 2021

View reviewed changes

kolyshkin mentioned this pull request Oct 25, 2021

Test fixes #87

Merged

thaJeztah closed this in #87 Oct 27, 2021

thaJeztah mentioned this pull request Oct 17, 2022

Go SDK: Move to its own go.mod and vanity URL dagger/dagger#3416

Closed

thaJeztah deleted the test_tip_tip branch June 23, 2025 12:41

	test: RUN_VIA_SUDO = $(shell $(SUDO) true && echo -exec \"$(SUDO)\")
	test test-local: RUN_VIA_SUDO = $(shell $(SUDO) true && echo -exec \"$(SUDO)\")

mount: also test against local mountinfo source code #68

mount: also test against local mountinfo source code #68

Conversation

thaJeztah commented Mar 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 9, 2021

Uh oh!

kolyshkin commented Mar 13, 2021

Uh oh!

thaJeztah commented Apr 6, 2021

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!

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!

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!

kolyshkin commented Sep 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Oct 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants