diff --git a/pkg/builder/eest_payloads.go b/pkg/builder/eest_payloads.go index d4433ae..4418a47 100644 --- a/pkg/builder/eest_payloads.go +++ b/pkg/builder/eest_payloads.go @@ -255,18 +255,26 @@ func (b *EESTPayloadsBuilder) Build(ctx context.Context, name string, opts Build return false, err } - if err := b.run(ctx, log, target); err != nil { - return false, err - } + runErr := b.run(ctx, log, target) // Record the config fingerprint (computed pre-run) for a later - // --rebuild-on-diff run. Best-effort; a failure must not fail the build. + // --rebuild-on-diff run. Written even when the fill failed: fill-stateful + // continues through individual test failures and still writes every fixture + // it did produce, so a target where 1 of 100 tests failed to fill must still + // be described on disk (the build summary reads this sidecar for the EEST + // repo, tests and marker). An output_dir holding only sidecars still counts + // as unpopulated, so this can't make a later build skip a target that + // produced nothing. Best-effort; a failure here must not mask runErr. if inputsErr != nil { log.WithError(inputsErr).Warn("Failed to compute build fingerprint; sidecar not written") } else if err := writeBuildSidecar(target.OutputDir, EESTPayloadsBuilderName, inputs); err != nil { log.WithError(err).Warn("Failed to write build fingerprint sidecar") } + if runErr != nil { + return false, runErr + } + return false, nil } @@ -828,12 +836,12 @@ func (b *EESTPayloadsBuilder) buildFillImage(ctx context.Context, log logrus.Fie const ( // eestFillResultFile is the sidecar written next to the fixtures recording // how many tests the fill produced/failed (read by the build markdown summary). - eestFillResultFile = ".benchmarkoor-fill.json" + eestFillResultFile = sidecarPrefix + "fill.json" // pytestReportFile is the pytest-json-report output written under output_dir // (via PYTEST_ADDOPTS --json-report); it carries the authoritative // passed/failed tally. Relative to output_dir (= the fill container's /out). - pytestReportFile = ".benchmarkoor-pytest-report.json" + pytestReportFile = sidecarPrefix + "pytest-report.json" ) // dirSize returns the total size in bytes of all regular files under dir. diff --git a/pkg/builder/eest_payloads_test.go b/pkg/builder/eest_payloads_test.go index aa77377..830d7b2 100644 --- a/pkg/builder/eest_payloads_test.go +++ b/pkg/builder/eest_payloads_test.go @@ -376,6 +376,38 @@ func TestEESTPayloadsBuilder_BuildSkipsPopulatedDir(t *testing.T) { assert.FileExists(t, filepath.Join(dir, eestFillResultFile)) } +// A build that failed before producing any fixture still leaves its sidecars +// behind (the fingerprint is now written even when the fill errors). Those must +// not make the next build skip the target as "already populated". +func TestEESTPayloadsBuilder_BuildDoesNotSkipSidecarOnlyDir(t *testing.T) { + t.Setenv("SCHELK_STATE", filepath.Join(t.TempDir(), "absent.json")) + + dir := t.TempDir() + for _, name := range []string{buildSidecarFile, eestFillResultFile} { + require.NoError(t, os.WriteFile(filepath.Join(dir, name), []byte("{}"), 0o600)) + } + + cfg := &config.EESTPayloadsConfig{ + FillImage: "fill:latest", + // A local, non-existent repo path keeps the lazy EEST SHA resolution + // offline: `git ls-remote` fails immediately instead of hitting the network. + EESTRepo: filepath.Join(t.TempDir(), "no-such-repo"), + Targets: []config.EESTPayloadTarget{{ + Name: "compute", FillerClient: "geth", SourceDir: filepath.Join(t.TempDir(), "missing"), + OutputDir: dir, Fork: "Osaka", FillerImage: "geth:master", + Tests: []string{"tests/benchmark/compute"}, + }}, + } + + b := NewEESTPayloadsBuilder(noopLogger(), cfg, "docker", &fakeMgr{}, t.TempDir()) + + // Not skipped: it proceeds into the build and fails on the absent source_dir. + skipped, err := b.Build(context.Background(), "compute", BuildOptions{}) + require.Error(t, err) + assert.False(t, skipped, "a sidecar-only output_dir must not count as built") + assert.Contains(t, err.Error(), "source_dir") +} + func TestMaterializeAddressStubs(t *testing.T) { t.Setenv("TMPDIR", t.TempDir()) diff --git a/pkg/builder/fingerprint.go b/pkg/builder/fingerprint.go index 5721ac9..a3acfb0 100644 --- a/pkg/builder/fingerprint.go +++ b/pkg/builder/fingerprint.go @@ -15,7 +15,7 @@ import ( // at an output_dir root after a successful build. When --rebuild-on-diff is set, // the next build compares the current config fingerprint against this file to // decide whether the output is stale and must be rebuilt. -const buildSidecarFile = ".benchmarkoor-build.json" +const buildSidecarFile = sidecarPrefix + "build.json" // buildFingerprintSchema versions the sidecar format so a future change can // invalidate old sidecars deliberately. diff --git a/pkg/builder/util.go b/pkg/builder/util.go index a6a6326..b48b3ec 100644 --- a/pkg/builder/util.go +++ b/pkg/builder/util.go @@ -8,6 +8,7 @@ import ( "io" "os" "path/filepath" + "strings" "sync" "time" @@ -28,8 +29,18 @@ func mountTempDir() string { return os.TempDir() } -// isPopulated reports whether dir exists and contains at least one -// entry. A missing dir returns (false, nil). +// sidecarPrefix marks the bookkeeping files benchmarkoor itself writes into an +// output_dir (the build fingerprint, the eest fill result, the pytest report). +// They describe a build rather than being its product. +const sidecarPrefix = ".benchmarkoor-" + +// isPopulated reports whether dir exists and holds at least one entry a builder +// actually produced. A missing dir returns (false, nil). +// +// benchmarkoor's own sidecars don't count: they are written even when a build +// fails before producing anything, so a dir holding nothing else must still read +// as empty — otherwise the next build would skip it as "already populated" and +// the target would never be produced. func isPopulated(dir string) (bool, error) { entries, err := os.ReadDir(dir) if err != nil { @@ -40,7 +51,13 @@ func isPopulated(dir string) (bool, error) { return false, fmt.Errorf("reading output_dir %q: %w", dir, err) } - return len(entries) > 0, nil + for _, entry := range entries { + if !strings.HasPrefix(entry.Name(), sidecarPrefix) { + return true, nil + } + } + + return false, nil } // prepareOutputDir ensures dir exists. When force is true the directory is diff --git a/pkg/builder/util_test.go b/pkg/builder/util_test.go index 5f7201d..ae4942c 100644 --- a/pkg/builder/util_test.go +++ b/pkg/builder/util_test.go @@ -1,12 +1,52 @@ package builder import ( + "os" + "path/filepath" "sync" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +func TestIsPopulated(t *testing.T) { + t.Run("missing dir is not populated", func(t *testing.T) { + got, err := isPopulated(filepath.Join(t.TempDir(), "absent")) + require.NoError(t, err) + assert.False(t, got) + }) + + t.Run("empty dir is not populated", func(t *testing.T) { + got, err := isPopulated(t.TempDir()) + require.NoError(t, err) + assert.False(t, got) + }) + + // A build that failed before producing anything still writes its sidecars. + // They must not make the dir look built, or the next build would skip it. + t.Run("sidecars alone are not populated", func(t *testing.T) { + dir := t.TempDir() + for _, name := range []string{buildSidecarFile, eestFillResultFile, pytestReportFile} { + require.NoError(t, os.WriteFile(filepath.Join(dir, name), []byte("{}"), 0o600)) + } + + got, err := isPopulated(dir) + require.NoError(t, err) + assert.False(t, got) + }) + + t.Run("any produced entry alongside sidecars is populated", func(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, buildSidecarFile), []byte("{}"), 0o600)) + require.NoError(t, os.MkdirAll(filepath.Join(dir, ".meta"), 0o755)) + + got, err := isPopulated(dir) + require.NoError(t, err) + assert.True(t, got) + }) +} + // TestTailBufferConcurrent exercises the case the mutex guards: the container // log-streaming goroutine keeps Write()-ing while the caller reads String() on // the error path (RunInitContainer doesn't join the streaming goroutine). Run