test: Add downstream tests against real external consumers (joern, databricks/dicer)#1868
test: Add downstream tests against real external consumers (joern, databricks/dicer)#1868bor-p-s wants to merge 19 commits into
Conversation
Adds test_dogfood.sh, cloning independently-maintained public consumers (lila, joern, databricks/dicer) at pinned commits and building a target against this checkout via local_path_override, to catch consumer-breaking changes before they ship. Each test function is `_`-prefixed so it's skipped by default (run explicitly via RULES_SCALA_TEST_ONLY) and not yet wired into .bazelci/presubmit.yml, since cloning + fetching Maven deps for several external repos on every PR is slow and network-dependent.
"Dogfooding" implies we control the dependent project; these build real, independently-maintained external repos we don't control, so "downstream" is the more accurate term throughout (file names, function names, comments). Also drops the lila and databricks/dicer test functions for now, keeping only joern -- it needed the least consumer-specific patching of the three (no Scala-version override, no Maven repin, just a git remote rewrite), making it the simplest starting point. The generic _downstream_build helper is otherwise unchanged and still designed to grow more consumers later.
A plain build only proves the code still compiles under this checkout; it says nothing about whether rules_scala's test-running machinery (discovery, the scala_test/scala_junit_test runner, etc.) still behaves correctly at runtime. Switch _downstream_build from `bazel build` to `bazel test`, and add joern's //semanticcpg:tests target alongside the library so its own test suite actually runs against this checkout.
…usions Switches from a hand-picked //semanticcpg allowlist to //... with explicit negative patterns for the few targets that fail for reasons that have nothing to do with rules_scala (php2cpg needs a real `php` binary, rust2cpg needs cargo/rustc, swiftsrc2cpg times out on its own native toolchain setup). A broad pattern with explicit exclusions catches regressions in code we didn't think to allowlist and keeps covering new joern code automatically, rather than silently stopping at whatever we happened to pick. Also fixes javasrc2cpg's tests, which write to ~/.shiftleft outside the sandbox: pre-create that directory and pass --sandbox_writable_path so the write succeeds instead of failing with "Operation not permitted". Verified: all 9 remaining test targets (dataflowengineoss, c2cpg, javasrc2cpg, jssrc2cpg, kotlin2cpg, pysrc2cpg, rubysrc2cpg, x2cpg, semanticcpg) pass end-to-end via the actual script (~266s).
joern (across its entire tree) only ever uses scala_library/scala_test, pinned to Scala 3.7.4 -- it never exercises rules_scala's scala_proto/ ScalaPB (gRPC) codegen path, and provides zero Scala 2.x coverage. dicer exercises both, so it's added back (dropped earlier when trimming the initial multi-consumer version down to joern only) as a second, genuinely non-overlapping consumer, not redundant with joern's coverage. Uses the same //... -minus-known-bad-exclusions approach as joern rather than a hand-picked target list. The exclusions here (caching/util:etcd_suite, dicer/common:etcd_bootstrapper_test, all of dicer/assigner/...) all fail because a downloaded etcd release binary exits with code 126 on this machine -- looks like a missing darwin-arm64 build, plausibly fine on a Linux CI runner, so worth re-checking there rather than assuming it holds. Verified: 112 of 112 remaining test targets pass end-to-end via the actual script (~143s).
No-op by default (every test in the file is _-prefixed and skipped), so this costs nothing on ordinary PRs. Rebuild the step with RULES_SCALA_TEST_REGEX=_test_downstream set as a custom environment variable (Buildkite's "New Build" dialog) to actually run it against a given commit -- no separate pipeline needed.
Expands the header comment: how to rebuild the presubmit step in Buildkite with RULES_SCALA_TEST_REGEX=_test_downstream to actually run it against a given commit, and a pointer to _downstream_build's Args for adding a new consumer.
The root wrapper was a content-free stub pointing at test/shell/test_downstream.sh, matching every sibling test_*.sh -- but unlike those, this one isn't self-explanatory from its name alone. Add a short summary (what it is, how to run it locally and in CI) so it isn't lost on whoever opens the root file first, without duplicating the fuller docs (Args, adding a consumer) that stay next to the implementation.
WojciechMazur
left a comment
There was a problem hiding this comment.
Can we also try to make them a dedicated Bazel-native tests? It would follow the current progress in bazelfication of tests
| - "./test_examples.sh" | ||
| downstream_linux: | ||
| # Every test in test_downstream.sh is skipped by default (see its own | ||
| # comments). Rebuild this step with RULES_SCALA_TEST_REGEX=_test_downstream |
There was a problem hiding this comment.
Would it be possible to use Buildkite-native constructs to keep the build conditional/manual?
As in, not gating every test on an env var, but instead gating the entire step on an approval via UI (?)
|
Thanks for your reviews, I'll refactor. |
…test) Replaces the bash-based test_downstream.sh harness with real Bazel constructs, per review feedback on PR #1868 (Wojciech, Paweł), and matching this repo's own ongoing migration of shell-driven tests to bazel_test-style macros. - test/community_build/downstream_repository.bzl: a repository_rule that fetches a consumer via git at a pinned commit, resolves this checkout's absolute path via repository_ctx.path(Label(...)) instead of computing it in bash, and writes local_path_override directly into the consumer's MODULE.bazel. Exposed through a downstream_consumers module_extension (matching this file's existing scala_deps/dev_deps convention) so MODULE.bazel only declares name/remote/commit/scala_version per consumer -- consumer-specific patching (e.g. dicer's protobuf-version override) and the pin's `scala_version` (generated into a loadable version.bzl) both live in the .bzl file, not scattered across MODULE.bazel and BUILD. - test/community_build/downstream_test.bzl + downstream_test_driver.sh: wraps a nested `bazel test` against the fetched repo as a real sh_test, reusing test/expect_build_failure/nested_bazel.sh's helpers. Tagged "manual" (Bazel's own opt-in mechanism, replacing the old RULES_SCALA_TEST_REGEX convention -- excluded from `bazel test //...` and from wildcard patterns like `:all`/`...` alike, run by naming targets explicitly) and "external" (disables result caching: the nested build reads this checkout's live source tree, so rules_scala's own sources are code under test without being declared sh_test inputs -- without "external" a cached PASS would survive edits to them). - A marker package the repository rule generates itself lets the sh_test force the fetch via a real Bazel dependency edge without triggering package-load failures on the consumer's own BUILD/bzl files (which reference deps not in our module graph, e.g. joern's aspect_rules_lint). Two consumers, chosen to be non-redundant: joern (Scala 3, only scala_library/scala_test) and databricks/dicer (Scala 2.12, also exercises the scala_proto/ScalaPB codegen path joern never touches). Verified end-to-end repeatedly against both, from a clean cache, including a one-off mutation test (a deliberate throw injected into ScalacWorker.main) that went red in 44s and was reverted -- confirming the nested build actually compiles this checkout's live production code rather than passing for infrastructure reasons only. Along the way, found and fixed real bugs rather than working around them: modern runfiles trees omit the "external/" prefix $(rootpath) still includes; bazelisk prepends its already-resolved binary's directory to a test action's PATH (right for same-repo nested tests, wrong here since we need the *consumer's* pinned Bazel version), fixed by filtering that one entry out by content rather than assuming its position; a consumer's lockfile may not live at a hardcoded "maven_install.json" path (dicer's is at "bazel:maven_install.json"), fixed by parsing every `lock_file = "//..."` the consumer's own MODULE.bazel declares; and the repin step reliably failed silently inside the sandboxed test action because this driver script's own RUNFILES_DIR/JAVA_RUNFILES (pointing at *its own* test runfiles) leaked into the nested `bazel run @maven//:pin` -- itself an sh_binary resolving its data file via $(rlocationpath) -- confusing it into looking in the wrong runfiles tree; fixed by unsetting those vars before any nested invocation. Documents the coverage boundary in the new README: scala_junit_test, specs2, scala_library_suite, twitter_scrooge, scalafmt, semanticdb, and the coverage path have no live, publicly buildable Bzlmod consumer as of this writing (checked GitHub code search; the historical heavy users are closed-source or long unmaintained) and stay covered by this repo's own tests and examples/ instead. Removes the now-superseded bash version (test_downstream.sh, test/shell/test_downstream.sh, and its skip-by-default presubmit.yml task).
…zel //... test/community_build/BUILD loaded scala_version from a generated @<name>//_bazel_native_marker:version.bzl in each consumer repo. load() runs at package-load time regardless of the manual tag on the sh_test targets, so any wildcard invocation (bazel build/query //...) fetched both joern and dicer unconditionally -- this broke the analyze_all, ubuntu2004, macos and bcr_presubmit CI tasks, which all build/test //.... Drop that load() and the version.bzl/marker plumbing behind it; declare scala_version as a literal in BUILD instead, kept in sync with the MODULE.bazel pin by hand. Verified bazel query '//... -//test_expect_failure/...' (CI's exact pattern) no longer fetches either consumer, while joern_test/dicer_test still resolve and dicer_test still passes end to end.
test_lint.sh's check_module_bazel MODULE.bazel test runs `bazel mod tidy` and diffs it against the committed file -- it wants tag attributes other than `name` alphabetized (commit before remote) and use_repo() args alphabetized (dicer before joern). Verified `bazel mod tidy` now produces no diff, and ./test_lint.sh passes end to end.
native.sh_test is gone on Bazel 10 (last_green's version), so test/community_build failed to load there, breaking any test_rules_scala.sh wildcard touching test/... (soft_fail-tagged, so it didn't block the build, but was a real regression). Switch to rules_shell's sh_test, matching expect_build_failure.bzl. Verified with USE_BAZEL_VERSION=last_green.
Adds downstream_block to presubmit.yml: uploads a "block" step so test/community_build's tests (slow, network-dependent) can be run from a PR's Buildkite UI instead of on every build. The actual task lives in .bazelci/downstream.yml so it's never auto-run.
| # be triggered manually instead. presubmit.yml's `downstream_block` task | ||
| # uploads a Buildkite "block" step wired to run this task's `runner_step` | ||
| # only once someone clicks it; this file is never read by the normal, | ||
| # always-run pipeline generation. |
There was a problem hiding this comment.
Could you show screenshots of how running it looks like now?
| name: "Downstream tests (joern, dicer)" | ||
| platform: ubuntu2004 | ||
| shell_commands: | ||
| - mv tools/bazel.rc.buildkite tools/bazel.rc |
There was a problem hiding this comment.
Btw! #1869 you can check it at some point (low prio tho)
| # `etcd_release` binary (`//bazel:etcd.bzl`) that exits with code 126 on this | ||
| # machine -- looks like a missing darwin-arm64 build of that specific etcd | ||
| # release, an environment limitation, not rules_scala's concern. Plausibly | ||
| # fine on a Linux CI runner; worth re-checking there rather than assuming |
There was a problem hiding this comment.
Hmmm is it a implicit TODO? 🤔
Also, aren't we already running on Linux runner in CI? so maybe it's easy to recheck now?
This reverts commit 985353e.
| ["git", "fetch", "--quiet", "--depth", "1", "origin", commit], | ||
| timeout = 600, | ||
| ) | ||
| if result.return_code != 0: |
There was a problem hiding this comment.
Wow I'm surprised that Bazel doesn't just fail in such case 😯
As in, I'm used to fail-fast, no exception handling in Bazel
There was a problem hiding this comment.
(no action required ofc)
There was a problem hiding this comment.
Confirmed empirically out of curiosity: wrote a throwaway repository_rule that runs a command with a non-zero exit and skips the check — bazel query completes without complaint. repository_ctx.execute() really does just hand you the exit code and leave fail-fast up to you.
| extra_bazel_flags, | ||
| ] + targets, | ||
| data = [ | ||
| "@{}//_bazel_native_marker:marker.txt".format(repo_name), |
There was a problem hiding this comment.
Wow indeed no other edges to the repo 😯 I would guess some concrete dep would be used instead
Ofc it works, keep it so
- downstream_test_driver.sh: named --flags instead of positional args (Bazel mangles blank args; matches expect_build_failure.sh's convention) - Move dicer's patch into MODULE.bazel's consumer(...) call as a real .patch file, instead of a centralized awk-based dict - Derive git-fetch timeout from measured cost (20s, not a round number) - Drop dicer's etcd exclusion (linux-amd64-only failure, not CI's concern)
| # linux-amd64 release, so it should run fine on this repo's own linux-amd64 | ||
| # CI -- it only fails locally, with "etcd exited with code 126", on other | ||
| # architectures (e.g. darwin-arm64). Unconfirmed on real CI, though: this | ||
| # target has no active CI trigger yet (still `manual`, see README). |
There was a problem hiding this comment.
🤔 blocked on permissions?
There was a problem hiding this comment.
Or will the PR need to merged first before it can be first tested?
Verified on real CI (branch downstream-test-etcd-verify, builds #6358-#6363): SSH->HTTPS git rewrite via XDG_CONFIG_HOME (older CI git predates GIT_CONFIG_GLOBAL), sandbox_writable_path for joern's coursier downloads, eternal sh_test timeout for both downstream tests, and restricting them to linux-only (macOS times out at the edge of its own budget; dicer's etcd binary is linux-amd64 only).
| fi | ||
|
|
||
| REPIN=1 nested_bazel_run run --repo_env=SCALA_VERSION="${scala_version}" @maven//:pin | ||
|
|
There was a problem hiding this comment.
Wow the amount of stuff that's needed to make it pass is overwhelming 🤯
| echo "${lockfile} missing (or empty) after repin -- treating as a real failure" >&2 | ||
| exit 1 | ||
| fi | ||
| done < <(grep -oE 'lock_file = "//[^"]*"' MODULE.bazel | sed -E 's/lock_file = "(.*)"/\1/') |
There was a problem hiding this comment.
Nit: prefer ... | while ..., it reads more logically (input-then-output, not output-then-input)
PawelLipski
left a comment
There was a problem hiding this comment.
That was a wild ride, approving!
|
(ofc subject to making sure these tests actually pass in CI 😅) |
Declaring the actual size is more honest than overriding timeout on a smaller declared size.
extra_bazel_flags only bumped the moderate bucket (300->900), leaving x2cpg/kotlin2cpg (already "large", 900s default) with no real margin -- that's what was actually timing out on macOS, not a platform incompatibility. Bump moderate and long together to 1800 instead of restricting to linux. Also trim stale narrative from comments.
| name = "joern_test", | ||
| repo_name = "joern", | ||
| scala_version = "3.7.4", | ||
| extra_bazel_flags = "--test_timeout=60,1800,1800,3600", |
There was a problem hiding this comment.
Huh but why 4 distinct timeouts? how does Bazel even apply them?
There was a problem hiding this comment.
--test_timeout takes one value (applies to every size) or exactly four, positional for short/moderate/long/eternal (small/medium/large/enormous). Passing four overrides those buckets' 60/300/900/3600s defaults for this one nested bazel test run. jssrc2cpg is medium, x2cpg/kotlin2cpg are large, both sitting right at their bucket's edge under CI contention. Bumping moderate and long together fixes both without touching joern's own BUILD files.
Windows build #6367 failed: the driver's cp -R can't recreate the symlinks in joern's own tree (CLI wrappers, frontend binaries) without elevated privileges. Unrelated to the earlier macOS timeout issue, so scope the incompatibility to windows specifically via select().
| extra_bazel_flags = "--test_timeout=60,1800,1800,3600", | ||
| size = "enormous", | ||
| target_compatible_with = select({ | ||
| "@platforms//os:windows": ["@platforms//:incompatible"], |
There was a problem hiding this comment.
Also, weren't some tests incompatible with macOS as well?

Description
test_downstream.shclones joern and databricks/dicer at pinned commits andruns their test suites against this rules_scala checkout via
local_path_override.Motivation
Part of #1866. joern (Scala 3, only
scala_library/scala_test) and dicer(Scala 2.12, plus
scala_proto/gRPC) cover different parts of rules_scala.Wired into
.bazelci/presubmit.ymlas a skip-by-default step: it costsnothing on ordinary PRs, and rebuilding it with
RULES_SCALA_TEST_REGEX=_test_downstreamset runs it against that commit.