dropbox / dbx_build_tools

Dropbox's Bazel rules and tools
Other
208 stars 36 forks source link

Support test_suite for dbx_services_test #25

Closed olschofkah closed 2 years ago

olschofkah commented 3 years ago

Would it be possible to add support for test suites for the 'test' invoked on a dbx_services_test? The use case is that a collection of tests that can run in parallel may share a set of defined daemon or task services that need to be performed. Right now the following error occurs:

Traceback (most recent call last): File "/root/.cache/bazel/_bazel_root/7ebbdddc3c30692c56fb633c56be95c3/external/dbx_build_tools/build_tools/services/svc.bzl", line 481, column 75, in services_bin_impl launcher_args.append("--svc.test-bin={}".format(ctx.executable.bin.short_path)) Error: 'NoneType' value has no field or method 'short_path'

benjaminp commented 3 years ago

Traceback (most recent call last): File "/root/.cache/bazel/_bazel_root/7ebbdddc3c30692c56fb633c56be95c3/external/dbx_build_tools/build_tools/services/svc.bzl", line 481, column 75, in services_bin_impl launcher_args.append("--svc.test-bin={}".format(ctx.executable.bin.short_path)) Error: 'NoneType' value has no field or method 'short_path'

What are you doing to cause this error?

olschofkah commented 3 years ago

You'll see this error if you create a bazel test suite to wrap the test you're intending to run, and then pass that into the test field of dbx_services_test. Here's some pseudo code

py_test(
  name = "a-test-1",
  srcs = "//pycode:test.py",
  main = "//pycode:test.py",
)

py_test(
  name = "a-test-2",
  srcs = "//pycode:test2.py",
  main = "//pycode:test2.py",
)

test_suite(
  name = "the-test-suite",
  tests = [
    ":a-test-1",
    ":a-test-2",
  ],
)

dbx_services_test(
  name = "the-test-suite-with-shared-runtime-services",
  args = [...], # note here that the args are passed to the test and override args test itself ... which doesn't exactly work for a test suite concept as the args may be different per test. 
  test = ":the-test-suite",
  services = [...],
)
benjaminp commented 3 years ago

This isn't possible to support today because test_suite is intended only as a top-level rule by Bazel. For example, to do this, we'd have to be able to see the individual test targets in the test_suite, but test-suite just doesn't expose that.

olschofkah commented 3 years ago

I see. The test suite itself doesn't actually provide much value and only acts as a grouping of tests mechanic. The same problem could be solved by just allowing a collection of tests to be passed directly to dbx_services_test if it's only a problem of visibility.

dbx_services_test(
  name = "the-tests-with-shared-runtime-services",
  args = [...], 
  tests = [
    ":a-test-1",
    ":a-test-2",
  ],
  services = [...],
)

You'd likely not want to have an empty set of args override args set on the individual tests in this case. I'm not sure you really want to be doing that anyway? I found that a bit confusing.

Just as an fyi, I have tried the other direction ( putting multiple dbx_services_test in a test suite). It works as you'd expect with each of the services performing their work independently. There could be an approach here where for services, your DAG of services would reduce down to a single execution of the daemon/task. There are many reasons why you may not be able to concurrently run the same daemon/task in a single bazel execution ... such as ports being bound, cloud asset changes, non-sandbox file system changes, etc ... and running them sequentially may be expensive (it is in my case). I'd still assume you'd want to be able to opt into the current behavior though in the case of a test that mutates the state of the daemon running.

benjaminp commented 3 years ago

I see. The test suite itself doesn't actually provide much value and only acts as a grouping of tests mechanic. The same problem could be solved by just allowing a collection of tests to be passed directly to dbx_services_test if it's only a problem of visibility.

dbx_services_test(
  name = "the-tests-with-shared-runtime-services",
  args = [...], 
  tests = [
    ":a-test-1",
    ":a-test-2",
  ],
  services = [...],
)

Sure. Ultimately, this is going to boil down to running many executables within one Bazel test target. That's already possible today with a wrapper that invokes the individual tests, maybe with another rule to wrap it nicely.

Just as an fyi, I have tried the other direction ( putting multiple dbx_services_test in a test suite). It works as you'd expect with each of the services performing their work independently. There could be an approach here where for services, your DAG of services would reduce down to a single execution of the daemon/task. There are many reasons why you may not be able to concurrently run the same daemon/task in a single bazel execution ... such as ports being bound, cloud asset changes, non-sandbox file system changes, etc ... and running them sequentially may be expensive (it is in my case). I'd still assume you'd want to be able to opt into the current behavior though in the case of a test that mutates the state of the daemon running.

We run our tests in Linux mount and network namespaces, so these kinds of conflicts are not a problem with concurrent executions. And there are strong motivations to avoid sharing between tests such as undebuggable isolation violations.

jhance commented 2 years ago

Mis-understanding of how test_suite works, so closing.