importccl: Bazelify importccl test #56127

miretskiy · 2020-10-29T18:31:51Z

Make importccl test hermetic by using correct directories
and avoiding test data regeneration.

Use 16 shards for the test to speed it up.

Release Notes: None

cockroach-teamcity · 2020-10-29T18:32:02Z

This change is

irfansharif

LGTM, thanks for trying it out Yevgeniy! TIL.

(Irrelevant aside, from https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/73072807/Git+Commit+Messages, for our commit titles I think we generally don't "end the title with a period".)

pkg/ccl/importccl/BUILD.bazel

irfansharif · 2020-10-29T21:08:52Z

pkg/testutils/bazel.go

+
+// RunningUnderBazel returns true if the test is executed by bazel.
+func RunningUnderBazel() bool {
+	return os.Getenv(testSrcDirEnv) != ""


We've soured on using os.Getenv directly in favor or envutil.

irfansharif · 2020-10-29T21:12:41Z

pkg/testutils/bazel.go

+	// If testSrcDirEnv is not set, it means we are not running under bazel,
+	// and so we can use "" as our directory which should point to the
+	// src root.
+	return os.Getenv(testSrcDirEnv)


[nit] Mild preference for explicitly returning an empty string if not running under bazel. We could just piggy back our newly added RunningUnderBazel helper here. It would look similar to TestTempDir below.

irfansharif · 2020-10-29T21:16:15Z

DEPS.bzl

-        sum = "h1:KQqSlwJmTr7NmxtVOl6nPNSZQEk8XuLoLyNDYcmniBo=",
-        version = "v1.0.1-0.20200826112548-92602d883b11",
+        sum = "h1:p4AOBShzCogHTl1n5Ff16j5a24tvk1lc7fzRqduSpKM=",
+        version = "v1.0.1-0.20201022032720-3e27adf87325",


James will take a look, thanks for picking it up: #56128

pkg/testutils/bazel.go

miretskiy

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @irfansharif, @jlinder, and @miretskiy)

pkg/ccl/importccl/BUILD.bazel, line 110 at r2 (raw file):

Previously, otan (Oliver Tan) wrote…

i think gazelle only modifies some fields -- shard_count is not one of them given i don't see it as MergeableAttrs anywhere

https://sourcegraph.com/search?q=repo:%5Egithub%5C.com/bazelbuild/bazel-gazelle%24+shard_count&patternType=literal

Ahh.. Cool. I'll keep it off, but I'll leave the comment.

pkg/testutils/bazel.go, line 23 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

We've soured on using os.Getenv directly in favor or envutil.

Done.

pkg/testutils/bazel.go, line 45 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

[nit] Mild preference for explicitly returning an empty string if not running under bazel. We could just piggy back our newly added RunningUnderBazel helper here. It would look similar to TestTempDir below.

Done.

pkg/testutils/bazel.go, line 49 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Am I correct in understanding that for any test running under bazel, we'll eventually want to use TestTempDir instead of just TempDir?

Correct.

miretskiy

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @irfansharif, @jlinder, @miretskiy, and @otan)

pkg/testutils/bazel.go, line 45 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

Done.

Sorry... I decided to just use envutil directly... Don't like calling getenv on the same var if it can be helped.

miretskiy · 2020-10-30T13:05:38Z

FYI: I reverted the use of envutil in favor of OS due to failing tests: https://teamcity.cockroachdb.com/buildConfiguration/Cockroach_UnitTests/2405846?
It appears that envutil requries env variables be named with COCKROACH_ prefix. Clearly, that's not the case for bazel.

miretskiy · 2020-11-02T20:40:29Z

@irfansharif Can you take another look? I had to make some changes to bazel.go
In particular, I was having some issues w/ getting importccl tests to pass; I wound up adding TestDataPath method
to address this issue.

irfansharif

LGTM mod comments below. Thanks for breaking new ground here, I'm hopeful these testutils APIs will see further usage as we start measuring test times of other packages in earnest. +cc @alan-mas.

irfansharif · 2020-11-02T21:08:09Z

pkg/testutils/bazel.go

+	if v := os.Getenv(env); v != "" {
+		return v
+	}
+	panic("expected value for env " + env)


We generally avoid direct string concatenation (citations needed), in favor of something like

panic(errors.AssertionFailedf("missing env var: %s", env))

irfansharif · 2020-11-02T21:08:29Z

pkg/testutils/bazel.go

+// TestSrcDir returns the path to the "source" tree.
+//
+// If running under bazel, this will point to a private, *readonly*
+// directory containing symlinks (or copies) of the test data depencies.


s/depencies/dependencies

irfansharif · 2020-11-02T21:15:39Z

pkg/testutils/bazel.go

+// TestDataPath returns a path to the directory containing test data files.
+//
+// Test files are usually checked into the repository under "testdata" directory.
+// If we are not using bazel, then the test executes with current working directory of


"executes in the directory of the actual tests"

irfansharif · 2020-11-02T21:16:37Z

pkg/testutils/bazel.go

+// the actual test, so the files can be referenced via "testdata/subdir/file" relative path.
+//
+// However, if we are running under bazel, the data files are specified
+// via go_test "data" attribute.  These files, in turn, are available under run files directory.


"are available within the runfiles directory" (looks like it's one word: https://docs.bazel.build/versions/master/skylark/lib/runfiles.html)

irfansharif · 2020-11-02T21:19:08Z

pkg/testutils/bazel.go

+//
+// However, if we are running under bazel, the data files are specified
+// via go_test "data" attribute.  These files, in turn, are available under run files directory.
+// This helper attempts to construct appropriate path to the run files directory


`s/run files/runfiles/

irfansharif · 2020-11-02T21:20:29Z

pkg/testutils/bazel.go

+func TestDataPath(relative ...string) string {
+	if RunningUnderBazel() {
+		pieces := append(bazelRelativeTargetPath(), relative...)
+		return path.Join(TestSrcDir(), os.Getenv(testWorkspaceEnv), path.Join(pieces...))


Should we want to use requireEnv here for testWorkspaceEnv?

Feel free to ignore, but a comment reference to https://docs.bazel.build/versions/master/test-encyclopedia.html here would make this all a bit more friendly to future readers.

The initial working directory shall be $TEST_SRCDIR/$TEST_WORKSPACE.

irfansharif · 2020-11-02T21:37:07Z

pkg/testutils/bazel.go

+	// Strip out leading package "//"
+	target = strings.TrimPrefix(target, "//")
+
+	// Strip out ":XXX" from the last component


Why not split on ":", drop the last name, and then split on "/" to get the relative target path?

It would also make it easier to add a comment up top showing the transformation taking place here. If I've understood correctly, it's the following: //pkg/ccl/importccl:importccl_test -> pkg/ccl/importccl (?).

I was actually a bit surprised that this was not something provided out of the box, or at least easier. I assume we've looked at https://docs.bazel.build/versions/master/test-encyclopedia.html#location and found it inapplicable?

//foo/bar:unittest is the directory $(WORKSPACE)/$(BINDIR)/foo/bar/unittest.runfiles

Somewhat related, I couldn't find official documentation on it, but https://github.com/bazelbuild/bazel/blob/c266ac966761c4b3d8a408a03e407505c93effdd/tools/bash/runfiles/runfiles.bash#L57 has a reference to RUNFILES_DIR. I'm curious if that would help. Feel free to ignore though.

Add a bazel testutils file to help tests use correct paths. Release Notes: None

Make importccl test hermetic by using correct directories and avoiding test data regeneration. Use 16 shards for the test to speed it up. Release Notes: None

miretskiy

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @irfansharif, @jlinder, @miretskiy, and @otan)

pkg/testutils/bazel.go, line 34 at r4 (raw file):

Previously, irfansharif (irfan sharif) wrote…

We generally avoid direct string concatenation (citations needed), in favor of something like
panic(errors.AssertionFailedf("missing env var: %s", env))

Ty. Done.

pkg/testutils/bazel.go, line 40 at r4 (raw file):

Previously, irfansharif (irfan sharif) wrote…

s/depencies/dependencies

Done.

pkg/testutils/bazel.go, line 74 at r4 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Somewhat related, I couldn't find official documentation on it, but https://github.com/bazelbuild/bazel/blob/c266ac966761c4b3d8a408a03e407505c93effdd/tools/bash/runfiles/runfiles.bash#L57 has a reference to RUNFILES_DIR. I'm curious if that would help. Feel free to ignore though.

Re splitting: You're right -- i'm trying to transform //pkg/cmd/foo:bar -> pkg/cmd/foo.
I was worried about path containing multiple ":", but that would break many things...
So, yeah, thanks for the suggestion -- changed. I actually, don't even need to split on "/".
Just return that as a string ("pkg/ccl/...")

Yeah -- I tried not to use things not mentioned explicitly in the test encyclopedial.
Added large comment block with the above link.

pkg/testutils/bazel.go, line 89 at r4 (raw file):

Previously, irfansharif (irfan sharif) wrote…

"executes in the directory of the actual tests"

Done

pkg/testutils/bazel.go, line 93 at r4 (raw file):

Previously, irfansharif (irfan sharif) wrote…

"are available within the runfiles directory" (looks like it's one word: https://docs.bazel.build/versions/master/skylark/lib/runfiles.html)

Added large comment block on top to introduce the concept of RUNFILES.

pkg/testutils/bazel.go, line 94 at r4 (raw file):

Previously, irfansharif (irfan sharif) wrote…

`s/run files/runfiles/

Done.

pkg/testutils/bazel.go, line 99 at r4 (raw file):

https://docs.bazel.build/versions/master/test-encyclopedia.html
I could have used requireEnv... It's just for whatever reason, WORKSPACE is marked optional.
Though, I suppose, for all intents and purposes it should exist in our environment.
Changed to requireEnv.

irfansharif

Reviewed 1 of 5 files at r3, 1 of 4 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @irfansharif, @jlinder, @miretskiy, and @otan)

miretskiy · 2020-11-03T11:28:01Z

bors r+

craig · 2020-11-03T12:02:20Z

Build succeeded:

GitHub CI (Cockroach)

miretskiy requested review from irfansharif, adityamaru and a team October 29, 2020 18:31

irfansharif approved these changes Oct 29, 2020

View reviewed changes

irfansharif requested a review from jlinder October 29, 2020 21:22

miretskiy force-pushed the bazel branch from 44cc0ed to 5faf3d5 Compare October 29, 2020 21:36

blathers-crl bot requested a review from otan October 29, 2020 21:36

miretskiy requested a review from irfansharif October 29, 2020 21:36

miretskiy commented Oct 29, 2020

View reviewed changes

jlinder requested a review from a team October 29, 2020 21:48

miretskiy force-pushed the bazel branch from 5faf3d5 to 644f87c Compare October 29, 2020 21:49

miretskiy commented Oct 29, 2020

View reviewed changes

miretskiy force-pushed the bazel branch 2 times, most recently from df625e2 to ad0321a Compare October 30, 2020 13:04

miretskiy force-pushed the bazel branch 8 times, most recently from a6922ef to 45d13cb Compare November 2, 2020 19:58

irfansharif approved these changes Nov 2, 2020

View reviewed changes

Yevgeniy Miretskiy added 2 commits November 2, 2020 19:25

bazel: Add bazel testutils

db22d65

Add a bazel testutils file to help tests use correct paths. Release Notes: None

importccl: Bazelify importccl test

2c48656

Make importccl test hermetic by using correct directories and avoiding test data regeneration. Use 16 shards for the test to speed it up. Release Notes: None

miretskiy requested a review from irfansharif November 3, 2020 00:26

miretskiy commented Nov 3, 2020

View reviewed changes

miretskiy force-pushed the bazel branch from 45d13cb to 2c48656 Compare November 3, 2020 00:26

irfansharif approved these changes Nov 3, 2020

View reviewed changes

fixup! bazel: Add bazel testutils

2d1bd59

craig bot merged commit 3c9e7b6 into cockroachdb:master Nov 3, 2020

miretskiy mentioned this pull request Nov 4, 2020

bazel: unable to test pkg/ccl/importccl due to missing test data read permissions #56093

Closed

irfansharif mentioned this pull request Jan 15, 2021

build,tests: replace go test with with a new test runner that splits output in different files #59045

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

importccl: Bazelify importccl test #56127

importccl: Bazelify importccl test #56127

miretskiy commented Oct 29, 2020

cockroach-teamcity commented Oct 29, 2020

irfansharif left a comment

irfansharif Oct 29, 2020

irfansharif Oct 29, 2020

irfansharif Oct 29, 2020

miretskiy left a comment

miretskiy left a comment

miretskiy commented Oct 30, 2020

miretskiy commented Nov 2, 2020

irfansharif left a comment

irfansharif Nov 2, 2020

irfansharif Nov 2, 2020

irfansharif Nov 2, 2020

irfansharif Nov 2, 2020

irfansharif Nov 2, 2020

irfansharif Nov 2, 2020

irfansharif Nov 2, 2020

irfansharif Nov 2, 2020

irfansharif Nov 2, 2020

miretskiy left a comment

irfansharif left a comment

miretskiy commented Nov 3, 2020

craig bot commented Nov 3, 2020

importccl: Bazelify importccl test #56127

importccl: Bazelify importccl test #56127

Conversation

miretskiy commented Oct 29, 2020

cockroach-teamcity commented Oct 29, 2020

irfansharif left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miretskiy left a comment

Choose a reason for hiding this comment

miretskiy left a comment

Choose a reason for hiding this comment

miretskiy commented Oct 30, 2020

miretskiy commented Nov 2, 2020

irfansharif left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miretskiy left a comment

Choose a reason for hiding this comment

irfansharif left a comment

Choose a reason for hiding this comment

miretskiy commented Nov 3, 2020

craig bot commented Nov 3, 2020