Skip to content

Conversation

@stephenamar-db
Copy link
Collaborator

No description provided.

@stephenamar-db stephenamar-db merged commit 4034882 into master Jul 31, 2025
6 checks passed
@stephenamar-db stephenamar-db deleted the stephenamar-db/asserts branch July 31, 2025 21:27
stephenamar-db pushed a commit that referenced this pull request Aug 8, 2025
…est suites from running (#467)

This PR fixes an issue in the Mill build that was stopped
language/platform-specific suites like `FileTests` from being run,
leading to a loss of test coverage (notably, the missing tests include
the upstream golden file tests).

I'm not a mill expert and am unsure if this is an _optimal_ fix, but
empirically it seems to work based on the testing steps described below.

## The bug

Prior to #459, the JS and JVM
tests both ran test suites that were defined in language-specific source
directories (such as `FileTests`, which is defined in `src-js` and
`src-jvm-native` directories rather than the shared `src` dir):

```bash
$ ./mill 'sjsonnet.js[2.13.16].test' 2>&1 | grep -E "(FileTests|go_test_suite|test_suite)" | head -3
[141] Checking test_suite/arith_bool.jsonnet
[141] Checking test_suite/arith_float.jsonnet
[141] Checking test_suite/arith_string.jsonnet

$ ./mill 'sjsonnet.jvm[2.13.16].test' 2>&1 | grep -E "(FileTests|go_test_suite|test_suite)" | head -3
[117] + sjsonnet.BufferedRandomAccessFileTests.readChar 4ms  
[117] + sjsonnet.BufferedRandomAccessFileTests.readString 0ms  
[117] + sjsonnet.BufferedRandomAccessFileTests.readChar 0ms  
```

If we looked at these test targets' sources, we see that
language-specific source directories are included:

```bash
$ ./mill show 'sjsonnet.jvm[2.13.16].test.sources'
[
  "ref:v0:036fb0dd:/Users/joshrosen/Documents/oss/sjsonnet/sjsonnet/test/src",
  "ref:v0:3f3f627d:/Users/joshrosen/Documents/oss/sjsonnet/sjsonnet/test/src-jvm",
  "ref:v0:9b7be5ed:/Users/joshrosen/Documents/oss/sjsonnet/sjsonnet/test/src-jvm-native"
]

$ ./mill show 'sjsonnet.js[2.13.16].test.sources'
[
  "ref:v0:036fb0dd:/Users/joshrosen/Documents/oss/sjsonnet/sjsonnet/test/src",
  "ref:v0:6ad29257:/Users/joshrosen/Documents/oss/sjsonnet/sjsonnet/test/src-js",
  "ref:v0:c984eca8:/Users/joshrosen/Documents/oss/sjsonnet/sjsonnet/test/src-jvm-js"
]
```

After #459, which upgraded
the Mill version to 1.0.x, a different set of test source paths are
used:

```bash
$ ./mill show 'sjsonnet.jvm[2.13.16].test.sources'
[
  "ref:v0:036fb0dd:/Users/joshrosen/Documents/oss/sjsonnet/sjsonnet/test/src",
  "ref:v0:c984eca8:/Users/joshrosen/Documents/oss/sjsonnet/sjsonnet/test/src-2.13.16",
  "ref:v0:c984eca8:/Users/joshrosen/Documents/oss/sjsonnet/sjsonnet/test/src-2.13",
  "ref:v0:c984eca8:/Users/joshrosen/Documents/oss/sjsonnet/sjsonnet/test/src-2"
]

$ ./mill show 'sjsonnet.js[2.13.16].test.sources'
[
  "ref:v0:036fb0dd:/Users/joshrosen/Documents/oss/sjsonnet/sjsonnet/test/src",
  "ref:v0:c984eca8:/Users/joshrosen/Documents/oss/sjsonnet/sjsonnet/test/src-2.13.16",
  "ref:v0:c984eca8:/Users/joshrosen/Documents/oss/sjsonnet/sjsonnet/test/src-2.13",
  "ref:v0:c984eca8:/Users/joshrosen/Documents/oss/sjsonnet/sjsonnet/test/src-2"
]
```

These new paths still include the shared `src` path (so _some_, perhaps
_most_, tests continue to run), but the architecture-specific ones are
skipped:

```
$ ./mill 'sjsonnet.js[2.13.16].test' 2>&1 | grep -E "(FileTests|go_test_suite|test_suite)" | head -3
# no output
./mill 'sjsonnet.jvm[2.13.16].test' 2>&1 | grep -E "(FileTests|go_test_suite|test_suite)" | head -3
# no output
```

## Fix

I updated each `test` object / module (except for `client` and `server`,
which don't do any source directory customization and thus don't need
this fix) to explicitly list the source directories.

Empirically, this restores the old test source paths and the skipped
tests are now running.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants