Create better policy for test helper functions in Tast tests |
|||||
Issue descriptionI wrote this in https://chromium.googlesource.com/chromiumos/platform/tast/+/HEAD/docs/writing_tests.md : --- Global variables in Go are scoped at the package level rather than the file level: The scope of an identifier denoting a constant, type, variable, or function ... declared at top level (outside any function) is the package block. As such, all tests within a package like platform or ui share the same namespace. Please avoid adding additional identifiers to that namespace beyond the core test function that you pass to testing.AddTest. Place constants and helper functions within your test function: [example] If you need to share functionality between tests in the same package, please introduce a new descriptively-named subpackage; see e.g. the chromecrash package within the ui package, used by the ui.ChromeCrashLoggedIn and ui.ChromeCrashNotLoggedIn tests. --- I was (and still am) concerned that people will start adding constants, variables, and functions in their test files with generic names like "timeout" and "connect". Since the namespace is the entire package, which will include (potentially tens of) other tests, it's likely that we would have conflicts when two tests try to use the same name. I'm more worried that people will start making one test use identifiers declared in another. This could make it hard to delete or move tests between categories in the future, and it's also likely to make development painful (as you'll need to hunt down the location of a given identifier, although at least you'll know it's in the current directory). With the current guidelines, it's obvious where something is declared -- either it's in the test function or it's prefixed by a package name. The current blanket ban on non-test-function top-level identifiers (even if they're unexported) in test category packages is pretty painful, though. Splitting functionality out to separate functions often results in more-readable code, and needing to nest those functions in test functions can result in long, hard-to-follow test functions. It's also non-ideal to tell people to add a new subpackage for their test. It's reasonable if you have multiple tests covering the same feature that need to share code, but it feels gross if the package is only going to be used by a single test (see e.g. https://crrev.com/c/1213777). In the initial design of Tast, I avoided putting each test in its own package since doing so would require adding a new import statement in the test bundle's main.go file whenever a new test is added. It's easy to forget to do this (which would result in your test silently not running!) and it's also prone to merge conflicts (made more likely by CQ currently often taking multiple days to land changes). By instead grouping multiple tests into per-category packages, an import statement only needs to be added when a new category is added. I still don't have any great solutions for what we should do instead, but here are a few things that I should think about more: a) Maybe we could have the convention that any additional symbols that a test adds need to be prefixed via the test's name. For example, the ui.ChromeLogin test could declare functions like chromeLoginStartWebserver, chromeLoginLoadPage, and so on. It might be possible to validate the names using reflection in a unit test. The obvious downside to this is that it results in long, hard-to-type names. b) Maybe we could have the convention that all non-test top-level identifiers in a test category package need to live in a particular file, e.g. common.go. That'd make it easier to find things. Downsides to this are that the file could grow very large and that merge conflicts are common. Anything that I'm missing?
,
Sep 7
If there's a naming conflict, the compiler will let you know. :-) I'm more concerned about the potential for lots of weird inter-test dependencies in packages that contain dozens of tests (and the associated commit queue merge conflicts).
,
Sep 10
I don't like the idea (a) to prefix all shared constant/function names. We can't live without namespaces... The idea (b) sounds good to me. IIUC it means: - common.go must not contain tests. Its symbols can be referred from other files. - <others>.go must contain a test. Its symbols must not be referred from other files. Can we enforce these rules with presubmit or unit tests?
,
Sep 10
BTW, another (crazy) idea is to put everything inside init(). It looks unsane, but may resolve some problems.
func init() {
const timeout = 3 * time.Second
getPID := func(ctx context.Context) { .... }
TestMain := func(s *testing.State) {
...
}
testing.AddTest(&testing.Test{
Func: TestMain,
...
})
}
,
Sep 11
Putting everything in init() is an interesting idea. I think one downside is that the testing.AddTest() call will need to appear at the end of init() (since it's executed in-order), which makes it hard to see important info like the test's description and attributes. It's also the case that everything in init() will be indented 4 spaces, so it probably doesn't improve readability vs. just nesting things in the test function. And we still have the problem (which I forgot to mention above) that you can't declare methods on struct types that are nested within a function. I suspect that people still won't like common.go since it moves helper functions away from the code that calls them -- it's not much better (and is maybe worse?) than just introducing a new subpackage. I wonder how hard it'd be to write a test that verifies that a given file's unexported toplevel symbols aren't referenced from any other files in the package. We might be fighting the compiler at that point, though...
,
Sep 19
,
Oct 24
Now that we got tast-lint, I tried prototyping a checker to detect references to identifiers in other files within the same package, and it worked. Do you think we can lift the current restriction if we get such a checker?
,
Oct 24
Thanks for looking into adding that! Yes, trying this out sounds good to me. I suspect that we may have a land grab in test packages where someone uses a name like "timeout" early on and then other authors experience conflicts when trying to declare the same name in their test files, but I think that that's probably still better than what we have right now. Tangentially, I wonder if tast-lint should also enforce an ordering for the items in a test file to keep tests easy to read, e.g. init func exported test func unexported things I'm not sure about this, though.
,
Oct 25
Thanks! Then I will prepare the CL. I'm not sure of the order because we tend to declare constants at the top of files. I would rather prefer defining exported test func at the bottom, but it would be arguable...
,
Oct 31
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/tast/+/c162e321a806616472952a7dccc15d0756ae4925 commit c162e321a806616472952a7dccc15d0756ae4925 Author: Shuhei Takahashi <nya@chromium.org> Date: Wed Oct 31 15:17:34 2018 tast-lint: Checks inter-file references in test main files. TestMain check is removed and two new checks are added instead: 1. InterFileRefs: checks that test main files do not have inter-file references. 2. Exports: checks that test main files does not export anything but test main functions. Introduction of the InterFileRefs check finally allows to loosen the current unideal restriction that test main files should not have any top-level declarations but test main functions. In order to check inter-file references, we have to parse other Go files in the same directory to build the complete map of declared objects in a package. cachedParser is introduced for this purpose. BUG= chromium:882022 TEST=fast_build.sh -T TEST=run_lint.sh src/**/*.go Change-Id: I52f2ee33423d4513eaad5a8349bea6cd905b5bb6 Reviewed-on: https://chromium-review.googlesource.com/1297560 Commit-Ready: Shuhei Takahashi <nya@chromium.org> Tested-by: Shuhei Takahashi <nya@chromium.org> Reviewed-by: Shuhei Takahashi <nya@chromium.org> [add] https://crrev.com/c162e321a806616472952a7dccc15d0756ae4925/src/chromiumos/cmd/tast-lint/check/inter_file_refs.go [add] https://crrev.com/c162e321a806616472952a7dccc15d0756ae4925/src/chromiumos/cmd/tast-lint/check/inter_file_refs_test.go [add] https://crrev.com/c162e321a806616472952a7dccc15d0756ae4925/src/chromiumos/cmd/tast-lint/check/exports_test.go [delete] https://crrev.com/0dc046a801a2cdfe1ac3578a08ececb1e417b346/src/chromiumos/cmd/tast-lint/check/test_main_test.go [modify] https://crrev.com/c162e321a806616472952a7dccc15d0756ae4925/src/chromiumos/cmd/tast-lint/check/errors.go [modify] https://crrev.com/c162e321a806616472952a7dccc15d0756ae4925/src/chromiumos/cmd/tast-lint/git.go [delete] https://crrev.com/0dc046a801a2cdfe1ac3578a08ececb1e417b346/src/chromiumos/cmd/tast-lint/check/test_main.go [modify] https://crrev.com/c162e321a806616472952a7dccc15d0756ae4925/docs/writing_tests.md [modify] https://crrev.com/c162e321a806616472952a7dccc15d0756ae4925/src/chromiumos/cmd/tast-lint/check/common.go [modify] https://crrev.com/c162e321a806616472952a7dccc15d0756ae4925/src/chromiumos/cmd/tast-lint/main.go [add] https://crrev.com/c162e321a806616472952a7dccc15d0756ae4925/src/chromiumos/cmd/tast-lint/parser.go [add] https://crrev.com/c162e321a806616472952a7dccc15d0756ae4925/src/chromiumos/cmd/tast-lint/check/exports.go
,
Nov 1
Finally! I'll send PSA. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by jkardatzke@chromium.org
, Sep 7