Continuous integration failed to detect bug in solver Hackage benchmarks

[grayjay]

#6447 added a short run of the solver Hackage benchmark to CI to test that it can run without errors. However, I noticed a bug where the benchmark fails to run when the cabal config files have not been initialized (#9494). This issue is for checking whether CI is still correctly testing the benchmark.

[Mikolaj]

For a start, does the test run when testing cabal locally according to the relevant instructions?

[grayjay]

@Mikolaj Which instructions are you referring to? CONTRIBUTING.md doesn't mention the Hackage benchmark, though it probably should. The benchmark should be able to run locally after #9494.

[grayjay]

#9500 (8541514) showed that CI still passes when the benchmark calls die at the start.

[grayjay]

c332032 showed that CI fails when the benchmark doesn't compile.

EDIT: The step "Validate build" of the job "Validate ubuntu-latest ghc-9.2.8" failed, though all other "Validate" jobs were cancelled.

[grayjay]

5ca936b showed that CI passes when the unit tests have a failure. The three CI runs mean that CI builds the Hackage benchmark but doesn't correctly run it or its unit tests.

[grayjay]

I'm not sure how to debug the CI next. It looks like validate.sh still attempts to run the benchmarks when the --solver-benchmarks flag is specified:

cabal/validate.sh

Lines 210 to 211 in a1cbd89

	--solver-benchmarks)
	BENCHMARKS=true

cabal/validate.sh

Line 269 in a1cbd89

if $BENCHMARKS; then STEPS="$STEPS solver-benchmarks-tests solver-benchmarks-run"; fi

cabal/validate.sh

Lines 464 to 477 in a1cbd89

	step_solver_benchmarks_tests() {
	print_header "solver-benchmarks: test"
	CMD="$($CABALLISTBIN solver-benchmarks:test:unit-tests)"
	(cd Cabal && timed $CMD) || exit 1
	}
	step_solver_benchmarks_run() {
	print_header "solver-benchmarks: run"
	SOLVEPKG=Chart-diagrams
	CMD="$($CABALLISTBIN solver-benchmarks:exe:hackage-benchmark) --cabal1=$CABAL --cabal2=$($CABALLISTBIN cabal-install:exe:cabal) --trials=5 --packages=$SOLVEPKG --print-trials"
	(cd Cabal && timed $CMD) || exit 1
	}

The --solver-benchmarks flag is used in two places, in .docker/validate-8.8.4.dockerfile

cabal/.docker/validate-8.8.4.dockerfile

Line 76 in a1cbd89

RUN sh ./validate.sh --doctest --solver-benchmarks --complete-hackage -w ghc-8.8.4 -v

cabal/cabal-dev-scripts/src/GenValidateDockerfile.hs

Line 44 in a1cbd89

, pair "8.8.4" $ Z "ghc-8.8.4" "8.8.4-bionic" False True False True "--doctest --solver-benchmarks --complete-hackage"

and .github/workflows/validate.yml

cabal/.github/workflows/validate.yml

Line 39 in a1cbd89

GHC_FOR_SOLVER_BENCHMARKS: '9.2.8'

cabal/.github/workflows/validate.yml

Lines 112 to 114 in a1cbd89

	if [[ ${{ matrix.ghc }} == ${{ env.GHC_FOR_SOLVER_BENCHMARKS }} ]]; then
	FLAGS="$FLAGS --solver-benchmarks"
	fi

[Mikolaj]

EDIT: The step "Validate build" of the job "Validate ubuntu-latest ghc-9.2.8" failed, though all other "Validate" jobs were cancelled.

That's normal. When one job fails, others get cancelled. So this case is fine.

[Mikolaj]

Good detective work. Yes, I confirm in the raw log from https://github.com/haskell/cabal/actions/runs/7109262010/job/19402640597?pr=9494 that benchmarks are built for GHC 9.2.8. However, they are not tested nor run, because validate.sh is no longer run as whole, to execute all the steps, but instead validate.yml decides which steps to execute and calls validate.sh for each separately. Unfortunately, it seems somebody did not cover the benchmark test and run steps in validate.yml (nor the time-summary step that should come after; no idea if it's still useful).

So this looks like an omission in validate.yml and it's hard to detect because of the duplication with validate.sh, where the benchmark steps are not omitted. I think we should just add the relevant steps to validate.yml, running them conditionally, probably just with the if [[ ${{ matrix.ghc }} == ${{ env.GHC_FOR_SOLVER_BENCHMARKS }} ]]; condition.

[grayjay]

@Mikolaj Thanks for the advice! I made the change that you suggested in #9625, and now CI fails with the error about a missing config file from #9494.

[grayjay]

I tried introducing a failure into the benchmark unit tests now that #9625 has been merged (5655b89), and it caused the CI to fail, as expected.

[Mikolaj]

I tried introducing a failure into the benchmark unit tests now that #9625 has been merged (5655b89), and it caused the CI to fail, as expected.

Amazing! Success!