From d2727c01ba9d9f046831033aaa3ad8a1d57f0dc4 Mon Sep 17 00:00:00 2001 From: William Chargin Date: Wed, 5 Sep 2018 10:53:05 -0700 Subject: [PATCH] Fix insidious quoting bug in build test script (#772) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: This patch fixes a particularly sneaky bug. Our test script contains a literal backtick inside single quotes. This is generally not a problem, because backticks inside single quotes do nothing. But the contents of the single quotes are interpreted as Bash by our test runner, and at that time the single quotes are expanded to a command substitution. Therefore, `grep` is invoked as if writing grep -e "warning: running $(yarn backend)" at the CLI. This will actually invoke `yarn backend`! The magnificent aspect of this bug is that it both makes the test script slower by about ten seconds _and_ completely and silently defeats the assertion in which it’s contained. The output of `yarn backend` contains several blank lines. Therefore, one of the literal patterns to `grep` contains a blank line. This causes `grep` to match _every_ line in the error file, regardless of whether it is one of the intended messages. This patch is the 666th PR to SourceCred. In my opinion, it deserves this dubious honor. Test Plan: Note that `yarn test --full` works, but fails if one of the expected error message patterns is deleted or munged. Confirm the behavior by prepending `echo backend >>/tmp/log &&` to the `yarn backend` script in `package.json`, noting that the resulting log file contains four lines before this patch and two lines after it. (Don’t forget to delete/clear the log file before invocations.) Confirm the behavior of `grep` by writing: ```shell $ printf 'things went wrong!\n' >err $ printf 'wat\n\nwot\n' >patterns $ grep -vF -e "okay" -e "warn: `cat patterns`" err; echo $? 1 $ printf 'wat\nwot\n' >patterns # no empty line $ grep -vF -e "okay" -e "warn: `cat patterns`" err; echo $? things went wrong! 0 ``` wchargin-branch: fix-build-test-quoting --- sharness/test_build_static_site.t | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sharness/test_build_static_site.t b/sharness/test_build_static_site.t index 721ec34..581955a 100755 --- a/sharness/test_build_static_site.t +++ b/sharness/test_build_static_site.t @@ -152,7 +152,7 @@ run_build() { run '"${flags}"' 2>err && test_must_fail grep -vF \ -e "Removing contents of build directory: " \ - -e "warn: running `yarn backend`" \ + -e "warn: running \`yarn backend\`" \ -e "warn: if this offends you" \ -e "info: loading repository" \ err &&