Navigation Menu

Skip to content
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

cmd/go: go test quietly skips test if custom flag is defined #36527

Closed
jarifibrahim opened this issue Jan 13, 2020 · 2 comments
Closed

cmd/go: go test quietly skips test if custom flag is defined #36527

jarifibrahim opened this issue Jan 13, 2020 · 2 comments

Comments

@jarifibrahim
Copy link

What version of Go are you using (go version)?

$ go version
go version go1.13 linux/amd64

Does this issue reproduce with the latest release?

I guess. I haven't tried.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN="/home/ibrahim/Projects/go/bin"
GOCACHE="/home/ibrahim/.cache/go-build"
GOENV="/home/ibrahim/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/ibrahim/Projects/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build158215380=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I have a package that has one subpackage. Let's call the top package as foo and sub-package bar. The directory structure looks like

.
├── bar
│   └── bar_test.go
└── main_test.go

1 directory, 2 files

main_test.go and bar_test.go both have one test each.

cat main_test.go
package main
...
var manual = flag.Int("manual", 0, "")
func TestFoo(t *testing.T) {}
cat bar_test.go
package bar
...
func TestHelloWorld(t *testing.T) {
	// t.Fatal("not implemented")
}

If I try to list all the tests via

go test -v -list= ./...                                                              
=== RUN   TestFoo
--- PASS: TestFoo (0.00s)
PASS
ok  	github.com/jarifibrahim/foo	0.001s
=== RUN   TestHelloWorld
--- PASS: TestHelloWorld (0.00s)
PASS
ok  	github.com/jarifibrahim/foo/bar	0.001s

which looks as expected but if I run

go test -v -manual=1 -list= ./...

it doesn't show the test TestHelloWorld

=== RUN   TestFoo
--- PASS: TestFoo (0.00s)
PASS
ok  	github.com/jarifibrahim/foo	0.001s

I understand the syntax of go test is go test [build/test flags] [packages] [build/test flags & test binary flags] but go test -manual=1 ... command doesn't crash with an error. It continues to run the test on the top level package only. It seems like go test assumed that there's only one package and ran the tests.

What did you expect to see?

I expect go test -manual=... packageName to return an error saying package manual (or whatever) is incorrect. It shouldn't quietly skip some tests.

What did you see instead?

go test -manual=0 ./... ran only the top level tests and go test ignored all the tests in the sub package since the top level package had a custom flag defined.

@jayconrod
Copy link
Contributor

This is working as intended, though the behavior is a bit confusing.

-manual=1 is not a known build or test flag, and it can't be a package (starts with -), so -manual=1 and everything after that are passed in the list of arguments to each test executable. Since the list of packages is empty, go test tests the package in the current directory: go test and go test . are equivalent. That's consistent with other build commands.

I think the fix for this would mean reporting an error if an unrecognized flag is not preceded by explicit package arguments. That would be a breaking change, and commands like go test -manual=1 would not work. Personally, I use things like go test -u or go test -testwork all the time, so I expect many people would miss this. We don't interpret arguments passed to tests, so checking for arguments that look like packages at the end of the command line would not be good enough to differentiate the new behavior.

cc @bcmills who spent some time looking at test flags recently.

jarifibrahim pushed a commit to dgraph-io/badger that referenced this issue Jan 15, 2020
This could possibly be a bug in `go test` command
golang/go#36527 .

The `go test` command would skip tests in sub-packages if
the top-level package has a `custom flag` defined and the
sub-packages don't define it.

Issue golang/go#36527 (comment)
has an example of this.

This PR also removes the code from the test that would unnecessary
start a web server. I see two problems here
1. An unnecessary web server running.
2. We cannot run multiple tests are the same time since the second
    run of the test would try to start a web server and
    crash saying `port already in use`.
@bcmills
Copy link
Contributor

bcmills commented Jan 22, 2020

Duplicate of #35097

@bcmills bcmills marked this as a duplicate of #35097 Jan 22, 2020
jarifibrahim pushed a commit to dgraph-io/badger that referenced this issue Mar 5, 2020
This could possibly be a bug in `go test` command
golang/go#36527 .

The `go test` command would skip tests in sub-packages if
the top-level package has a `custom flag` defined and the
sub-packages don't define it.

Issue golang/go#36527 (comment)
has an example of this.

This PR also removes the code from the test that would unnecessary
start a web server. I see two problems here
1. An unnecessary web server running.
2. We cannot run multiple tests are the same time since the second
    run of the test would try to start a web server and
    crash saying `port already in use`.

(cherry picked from commit 5870b7b)
jarifibrahim pushed a commit to dgraph-io/badger that referenced this issue Mar 6, 2020
* Cast sz to uint32 to fix compilation on 32 bit (#1175)

`env GOOS=linux GOARCH=arm GOARM=7 go build` no longer fails with overflow.

Similar to commit fb0cdb8.

Signed-off-by: Christian Stewart <christian@paral.in>
(cherry picked from commit 465f28a)

* Fix commit sha for WithInMemory in CHANGELOG. (#1172)

(cherry picked from commit 03af216)

* Fix windows build (#1177)

Fix windows build and some deepsource warnings

(cherry picked from commit 0f2e629)

* Fix checkOverlap in compaction (#1166)

The overlap check in compaction would keep additional keys in case
the levels under compaction had overlap amongst themselves.
This commit fixes it.

This commit also fixes the `numVersionsToKeep` check. Without this
commit, we would end up keeping `2` versions of a key even when the
number of versions to keep was set to `1`. See
`level 0 to level 1 with lower overlap` test.

Fixes #1053

The following test fails on master but works with this commit
```go
func TestCompaction(t *testing.T) {
	t.Run("level 0 to level 1", func(t *testing.T) {
		dir, err := ioutil.TempDir("", "badger-test")
		require.NoError(t, err)
		defer removeDir(dir)

		// Disable compactions and keep single version of each key.
		opt := DefaultOptions(dir).WithNumCompactors(0).WithNumVersionsToKeep(1)
		db, err := OpenManaged(opt)
		require.NoError(t, err)

		l0 := []keyValVersion{{"foo", "bar", 3}, {"fooz", "baz", 1}}
		l01 := []keyValVersion{{"foo", "bar", 2}}
		l1 := []keyValVersion{{"foo", "bar", 1}}
		// Level 0 has table l0 and l01.
		createAndOpen(db, l0, 0)
		createAndOpen(db, l01, 0)
		// Level 1 has table l1.
		createAndOpen(db, l1, 1)

		// Set a high discard timestamp so that all the keys are below the discard timestamp.
		db.SetDiscardTs(10)

		getAllAndCheck(t, db, []keyValVersion{
			{"foo", "bar", 3}, {"foo", "bar", 2}, {"foo", "bar", 1}, {"fooz", "baz", 1},
		})
		cdef := compactDef{
			thisLevel: db.lc.levels[0],
			nextLevel: db.lc.levels[1],
			top:       db.lc.levels[0].tables,
			bot:       db.lc.levels[1].tables,
		}
		require.NoError(t, db.lc.runCompactDef(0, cdef))
		// foo version 2 should be dropped after compaction.
		getAllAndCheck(t, db, []keyValVersion{{"foo", "bar", 3}, {"fooz", "baz", 1}})
	})
}
```

(cherry picked from commit 0a06173)

* Remove the 'this entry should've caught' log from value.go (#1170)

Fixes - #1031
(There wasn't a bug to fix. The log statement shouldn't have been there)

This PR removes the warning message `WARNING: This entry should have
been caught.`. The warning message assumed that we will always find the
**newest key if two keys have the same version** This assumption is
valid in case of a normal key but it's **NOT TRUE** in case of
**move keys**.

Here's how we can end up fetching the older version of a move key if
two move keys have the same version.

```
It might be possible that the entry read from LSM Tree points to an
older vlog file. This can happen in the following situation. Assume DB
is opened with numberOfVersionsToKeep=1

Now, if we have ONLY one key in the system "FOO" which has been updated
3 times and the same key has been garbage collected 3 times, we'll have
3 versions of the movekey for the same key "FOO".
NOTE: moveKeyi is the moveKey with version i
Assume we have 3 move keys in L0.
- moveKey1 (points to vlog file 10),
- moveKey2 (points to vlog file 14) and
- moveKey3 (points to vlog file 15).

Also, assume there is another move key "moveKey1" (points to vlog
file 6) (this is also a move Key for key "FOO" ) on upper levels (let's
say level 3). The move key "moveKey1" on level 0 was inserted because
vlog file 6 was GCed.

Here's what the arrangement looks like
L0 => (moveKey1 => vlog10), (moveKey2 => vlog14), (moveKey3 => vlog15)
L1 => ....
L2 => ....
L3 => (moveKey1 => vlog6)

When L0 compaction runs, it keeps only moveKey3 because the number of
versions to keep is set to 1. (we've dropped moveKey1's latest version)

The new arrangement of keys is
L0 => ....
L1 => (moveKey3 => vlog15)
L2 => ....
L3 => (moveKey1 => vlog6)

Now if we try to GC vlog file 10, the entry read from vlog file will
point to vlog10 but the entry read from LSM Tree will point to vlog6.
The move key read from LSM tree will point to vlog6 because we've asked
for version 1 of the move key.

This might seem like an issue but it's not really an issue because the
user has set the number of versions to keep to 1 and the latest version
of moveKey points to the correct vlog file and offset. The stale move
key on L3 will be eventually dropped by compaction because there is a
newer version in the upper levels.
```

(cherry picked from commit 2a90c66)

* Avoid sync in inmemory mode (#1190)

This makes db.Sync() no-op when badger is running in in-memory mode.
The previous code would unnecessarily load up an atomic and
acquire locks.

(cherry picked from commit 2698bfc)

* Use fastRand instead of locked-rand in skiplist (#1173)

The math/rand package (https://golang.org/src/math/rand/rand.go) uses
a global lock to allow concurrent access to the rand object.

The PR replaces `math.Rand` with `ristretto/z.FastRand()`. `FastRand`
is much faster than `math.Rand` and `rand.New(..)` generators.

The change improves concurrent writes to skiplist by ~30%
```go
func BenchmarkWrite(b *testing.B) {
	value := newValue(123)
	l := NewSkiplist(int64((b.N + 1) * MaxNodeSize))
	defer l.DecrRef()
	b.ResetTimer()
	b.RunParallel(func(pb *testing.PB) {
		rng := rand.New(rand.NewSource(time.Now().UnixNano()))
		for pb.Next() {
			l.Put(randomKey(rng), y.ValueStruct{Value: value, Meta: 0, UserMeta: 0})
		}
	})
}
```
```
name      old time/op  new time/op  delta
Write-16   657ns ± 3%   441ns ± 1%  -32.94%  (p=0.000 n=9+10)
```

(cherry picked from commit 9d6512b)

* Add Jaegar to list of projects (#1192)


(cherry picked from commit 01a00cb)

* Run all tests on CI (#1189)

This could possibly be a bug in `go test` command
golang/go#36527 .

The `go test` command would skip tests in sub-packages if
the top-level package has a `custom flag` defined and the
sub-packages don't define it.

Issue golang/go#36527 (comment)
has an example of this.

This PR also removes the code from the test that would unnecessary
start a web server. I see two problems here
1. An unnecessary web server running.
2. We cannot run multiple tests are the same time since the second
    run of the test would try to start a web server and
    crash saying `port already in use`.

(cherry picked from commit 5870b7b)

* Improve write stalling on level 0 and 1

We don't need to stall writes if Level 1 does not have enough space.
Level 1 is stored on the disk and it should be okay to have more
number of tables (more size) on Level 1 than the `max level 1 size`.
These tables will eventually be compacted to lower levels.

This commit changes the following
- We no longer stall writes if L1 doesn't have enough space.
- We stall writes on level 0 only if `KeepL0InMemory` is true.
- Upper levels (L0, L1, etc) get priority in compaction (previously,
    level with higher priority score would get preference)

(cherry picked from commit 3747be5)

* Update ristretto to version  8f368f2 (#1195)

This commit pulls following changes from ristretto
```
git log c1f00be0418e...8f368f2 --oneline
```
```

8f368f2 (HEAD -> master) Fix DeepSource warnings
adb35f0 delete item immediately 
fce5e91 Support nil *Cache values in Clear and Close
4e224f9 Add .travis.yml 
8350826 Fix the way metrics are handled for deletions
99d1bbb (tag: v0.0.1) default to 128bit hashing for collision checks
1eea1b1 atomic Get-Delete operation for eviction
8d6a8a7 add cache test for identifying MaxCost overflows
ae09250 use the custom KeyToHash function if one is set
1dd5a4d #19 Fixes memory leak due to Oct 1st regression in processItems
```

(cherry picked from commit 82381ac)

* Support disabling the cache completely. (#1183) (#1185)

The cache can be disabled by setting `opt.MaxCacheSize=0`

(cherry picked from commit 7e5a956)

* Fix L0/L1 stall test (#1201)

The test `TestL0Stall` and `TestL1Stall` would never fail
because of errors in the manifest file. This commit fixes it.

(cherry picked from commit 0acb3f6)

* Disable compression and set ZSTD Compression Level to 1 (#1191)

This PR
- Disables compression. By default, badger does not use any compression.
- Set default ZSTD compression level to 1
    Level 15 is very slow for any practical use of badger.

```
no_compression-16              10	 502848865 ns/op	 165.46 MB/s
zstd_compression/level_1-16     7	 739037966 ns/op	 112.58 MB/s
zstd_compression/level_3-16     7	 756950250 ns/op	 109.91 MB/s
zstd_compression/level_15-16    1	11135686219 ns/op	   7.47 MB/s
```

(cherry picked from commit c3333a5)

* Add support for caching bloomfilters (#1204)

This PR adds support for caching bloom filters in ristretto.
The bloom filters and blocks are removed from the cache 
when the table is deleted.

(cherry picked from commit 4676ca9)

* Rework concurrency semantics of valueLog.maxFid (#1184) (#1187)

Move all access to `valueLog.maxFid` under `valueLog.filesLock`, while
all mutations happen either with writes stopped or sequentially under
valueLog.write.

Fixes a concurrency issue in `valueLog.Read` where the maxFid variable
and the `writableLogOffset` variable could point to two different log files.

(cherry picked from commit 3e25d77)

* Change else-if statements to idiomatic switch statements. (#1207)



(cherry picked from commit eee1602)

* Fix flaky TestPageBufferReader2 test (#1210)

Fixes #1197

The `TestPageBufferReader2` test would fail often because of an
`off-by-1` issue. The problem can be reproduced by setting `randOffset`
to the biggest number that randInt31n may return statically like:

```
    //randOffset := int(rand.Int31n(int32(b.length)))
    randOffset := int(int32(b.length-1))
```

This makes the problem reliably reproducible as the offset is now
pointing at EOF.

Thus changing the line to this should hopefully solve the problem:
`randOffset := int(rand.Int31n(int32(b.length-1

(cherry picked from commit c51748e)

* Replace t.Fatal with require.NoError in tests (#1213)

We are using the following pattern in tests that can be
replaced with `require.NoError(t, err)`.
```go
if err != nil {
    t.Fatal(err)
}
```

(cherry picked from commit 78d405a)

* Add missing package to README for badger.NewEntry (#1223)


(cherry picked from commit 8734e3a)

* Remove ExampleDB_Subscribe Test (#1214)

The test ExampleDB_Subscribe doesn't run reliably on appveyor.
This commit removes it.

(cherry picked from commit e029e93)

* Fix int overflow for 32bit (#1216)

- Fix tests for 32 bit systems
- Enable 32-bit builds on Travis

(cherry picked from commit bce069c)

* Update CHANGELOG for Badger 2.0.2 release. (#1230)

Co-authored-by: Ibrahim Jarif <ibrahim@dgraph.io>
(cherry picked from commit e908818)

* Fix changelog for v2.0.2

(cherry picked from commit b81faa5)

Co-authored-by: Christian Stewart <christian@paral.in>
Co-authored-by: Leyla Galatin <56415703+lgalatin@users.noreply.github.com>
Co-authored-by: Damien Tournoud <damien@platform.sh>
Co-authored-by: Martin Martinez Rivera <martinmr@users.noreply.github.com>
Co-authored-by: Dieter Plaetinck <dieter@plaetinck.be>
Co-authored-by: Apoorv Vardhan <vardhanapoorv@users.noreply.github.com>
jarifibrahim pushed a commit to dgraph-io/badger that referenced this issue Mar 12, 2020
This could possibly be a bug in `go test` command
golang/go#36527 .

The `go test` command would skip tests in sub-packages if
the top-level package has a `custom flag` defined and the
sub-packages don't define it.

Issue golang/go#36527 (comment)
has an example of this.

This PR also removes the code from the test that would unnecessary
start a web server. I see two problems here
1. An unnecessary web server running.
2. We cannot run multiple tests are the same time since the second
    run of the test would try to start a web server and
    crash saying `port already in use`.

(cherry picked from commit 5870b7b)
jarifibrahim pushed a commit to dgraph-io/badger that referenced this issue Mar 24, 2020
This could possibly be a bug in `go test` command
golang/go#36527 .

The `go test` command would skip tests in sub-packages if
the top-level package has a `custom flag` defined and the
sub-packages don't define it.

Issue golang/go#36527 (comment)
has an example of this.

This PR also removes the code from the test that would unnecessary
start a web server. I see two problems here
1. An unnecessary web server running.
2. We cannot run multiple tests are the same time since the second
    run of the test would try to start a web server and
    crash saying `port already in use`.

(cherry picked from commit 5870b7b)
@golang golang locked and limited conversation to collaborators Jan 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants