Mixed external and "internal" tests

[sagikazarmark]

When an external test refers to something in the "internal" (in this case: it's in the the package, not in _test) package in a _test.go file, Please is unable to build the external test package. (See the example in this repo: https://github.com/sagikazarmark/please-issues/tree/master/go-external-test)

For example:

go_test(
    name = "test_external",
    srcs = ["pkg_external_test.go", "pkg_test.go"],
    external = True,
    deps = [":pkg"],
)

The above rule sources pkg_test.go which is in the pkg package, whereas pkg_external_test.go is in pkg_test.

The build fails with the following error:

❯ plz build --shell //pkg:test_external
Build stopped after 530ms. 1 target failed:
    //pkg:_test_external#lib
Error building target //pkg:_test_external#lib: exit status 1
pkg/pkg_external_test.go, line 12, column 14: undefined: pkg.CompareWith
pkg/pkg_test.go, line 1, column 9: package pkg; expected pkg_test

It complains about the package not being external.

As a workaround, I tried building a separate library from the library source and the necessary _test.go files:

go_library(
    name = "pkg_test_external",
    srcs = ["pkg.go", "pkg_test.go"],
)

go_test(
    name = "test_external2",
    srcs = ["pkg_external_test.go"],
    external = True,
    deps = [":pkg_test_external"],
)

It fails with a different error, complaining about a missing import path:

❯ plz build --shell //pkg:test_external2
Build stopped after 560ms. 1 target failed:
    //pkg:_test_external2#lib
Error building target //pkg:_test_external2#lib: exit status 1
pkg/pkg_external_test.go, line 6, column 2: can't find import: "github.com/sagikazarmark/please-issues/go-external-test/pkg"

I traced the error back to the way import config is built and commented about it here: https://github.com/thought-machine/please/pull/1177#issuecomment-687793475

Possible solutions

The above workaround would be a great start for solving these issues. Actually, in general, I think it would be great if go_library allowed specifying every aspect of package naming.

Another solution could be adding an internal_srcs attribute to go_test and accept test files from the same package there.

[sagikazarmark]

Looks like the above workaround won't work either: https://github.com/sagikazarmark/please-go-modules/pull/8

It'll try to compile all files into one library which will obviously fail.

[Tatskaari]

This seems like an awkward thing to support. At present, we don't re-compile the internal package with the test srcs for external tests so when you depend on that library, you get the production version. I guess what you could do is depend on the internal lib of the go_test rule rather than the go_library rule. This is a bit tricky as the import config is generated in a post build step because we don't know what package the test package will be at parse time.

This all seems very fiddly. Do you think this is worth the effort or was it just something you noticed?

[sagikazarmark]

Unfortunately, this is something that we actively do. I could patch these packages, but we are looking at a couple dozens of them :\

I was wondering why my workaround didn't work actually. One reason is the import path thing I pointed out above. The other is that external forces a recompile of all test files? If I could just disable that and manually compile the "internal" library, I think it should work.

[Tatskaari]

Aha! It might be because of filter_srcs which I think will exclude the *_test.go files. Hopefully setting that to False should allow you to generate a go_library for the internal package with the test srcs.

This is a bit naff but to get this working properly is all just a little fiddly. For non-external tests we create a second version of the internal library with the test srcs included. When we do this, we don't actually generate an import config for it. We rely on a post-build function that reads the output for the test main code generation which figures out the test package name for us. We use that to override that package's import config to the test version of the package. Because this is done in a post-build, it's a little tricky to depend on it for external stuff.

Let me know if the filter_srcs thing helps.

[Tatskaari]

@sagikazarmark as we discussed offline, that workaround helps. There's other lower hanging fruit I can work on right now so I'm going to leave this for now.

[sagikazarmark]

So this one is a bit harder than I expected. I tried turning filter_srcs off and ended up with an entirely new issue:

Error building target //internal/helm:_integration_test#lib: exit status 1
internal/helm/envresolver_integration_test.go, line 25, column 2: internal compiler error: conflicting package heights 22 and 13 for path "github.com/banzaicloud/pipeline/internal/helm"

I'm not even sure where to start to describing it :D

So I have a package that has both external and "non-external" tests. Basically the same situation as above. Since I have test code in the "internal" package, I need to recompile it with the test files and use it in the external tests.

Just to make things more complicated: my internal package is actually a transitive dependency of the external test package, so when I try to compile the external test package, I receive the above error.

I'm not sure what's the good solution here. My guess is, the go build tool simply recompiles the package and uses it everywhere. Obviously, please cannot do that, because it depends on the original go_library target.

The only way to do that is if please build rules can have a "test" version and when tests are run, please uses that version instead of the main one. I'm not sure how feasible it is though? Maybe we could "cheat" and use select or a similar solution?

Something like this:

if test:
    # export the test target
else:
    # export the primary target

go_library(name = "primary_target", srcs = glob(["*.go"], exclude = ["*_test.go"]))

go_library(name = "test_target", srcs = glob(["*.go"], exclude = ["*_test.go"]) + ["some_test.go"])

[sagikazarmark]

Here is Bazel's approach: https://github.com/bazelbuild/rules_go/blob/master/go/core.rst#embedding

Haven't looked under the hood, but they do it exactly because of the internal/external test problem.

[stale[bot]]

This issue has been automatically marked as stale because it has not had any recent activity in the past 90 days. It will be closed if no further activity occurs. If you require additional support, please reply to this message. Thank you for your contributions.

[sagikazarmark]

Bump

[Tatskaari]

This might actually be possible with the please_go tool. Going to put this on the v17 backlog.

[stale[bot]]

This issue has been automatically marked as stale because it has not had any recent activity in the past 90 days. It will be closed if no further activity occurs. If you require additional support, please reply to this message. Thank you for your contributions.