[SR-4534] Validate that benchmark commits contain freshly generated test harness

[palimondo]

Previous ID	SR-4534
Radar	None
Original Reporter	@palimondo
Type	Improvement
Status	Closed
Resolution	Done

Additional Detail from JIRA

Votes	0
Component/s	Project Infrastructure
Labels	Improvement
Assignee	@palimondo
Priority	Medium

md5: e2233ef2b29210a9925bdadf563786d1

relates to:

• SR-4533 Using gyb with benchmarks

Issue Description:

As discussed on swift-dev, when adding new performance tests to the benchmark suit, contributors sometimes forget to regenerate the test harness using benchmark/scripts/generate_harness/generate_harness.py and instead manually edit the benchmark/CMakeLists.txt and benchmark/utils/main.swift.

@gottesmm suggested to "add a test to the validation suite that locally generates the gyb/harness files and performs a diff? This will ensure that at least the bots will catch if someone forgets to run the update."

[gottesmm]

Thanks Pavol I would also make sure that you can check if someone forgets to regenerate the tests using gyb as well. But that can wait if you want.

[palimondo]

Michael, So far, I took the liberty of putting the gyb subprocess.call into the generate_harness.py, as that step is a prerequisite for correctly generating the harness itself. But I'm lost with that validation test. I read the Testing.rst and looked at the validation-test/Python/bug-reducer.test-sh, but I don't understand what that does at all. I found 3 other test-sh that invoke diff, yet I have no clue how to proceed here. Any and all guidance appreciated!

[palimondo]

#8641

cc @lplarson

[gottesmm]

@palimondo

Few things here:

1. You need to modify generate_harness so that the variable script_dir can be specified from the command line.
2. Then the test should do something like:

// RUN: rm -rfv %t
// RUN: %swift_src_root/benchmarks/scripts/test_generate_harness.sh %swift_src_root %t
// REQUIRES: OS=macosx
// REQUIRES: asserts
// REQUIRES: CMAKE_GENERATOR=Ninja

The script test_generate_harness.sh just is the driver for the test. The %t argument that it takes in is a temporary directory that should be used instead of the scripts directory. A strawman implementation of the script:

#!/bin/bash

SWIFT_SRC_DIR="$1"
BENCHMARK_DIR="${SWIFT_SRC_DIR}/benchmarks"
SCRIPT_DIR="${BENCHMARK_DIR}/scripts"
TEMP_DIR="$2"

"${SCRIPT_DIR}/generate_harness.py" "--output-dir=${TEMP_DIR}"
for f in $(cd "${TEMP_DIR}" && find ./ -type f); do
    diff "${TEMP_DIR}/${f}" "${BENCHMARK_DIR}/${f}"
    if [[ $? -ne 0 ]]; then
       exit 1
    fi
done

But that is just a straw man, I didn't try it out.

NOTE I restricted the script to only working on OS X just for simplicity reasons. Since we have PR testing that is good enough to ensure that the criterion isn't violated.

[palimondo]

Michael, thanks for doing all the hard work with test driver script and lit test here for me. I think I have it working. I have verified the working of new generate_harness.py, test_generate_harness.sh. But I’m not sure how to run validation-test/benchmarks/generate-harness.test-sh…
Could you have a look on the PR?

[palimondo]

PR #8641 was merged to master. Marking Done.