sh_do_test: verbose trace + start to remove inferred output_file by wilzbach · Pull Request #8025 · dlang/dmd

wilzbach · 2018-03-13T15:12:45Z

Now that a dedicated Bash wrapper exists, a lot of boilerplate code can be removed from the bash scripts.

This is a start which saves an verbose trace of the bash log in .log and all stdout and stddev in output_file. A trap will print both output when an error occurs.

There's a lot more that can be done, see #7928 for ideas.

CC @marler8997 @CyberShadow

dlang-bot · 2018-03-13T15:12:47Z

Thanks for your pull request, @wilzbach!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

wilzbach · 2018-03-13T15:15:59Z

test/compilable/jsonNoOutFile.sh

-$DMD -c -od=${dir} -Xi=compilerInfo compilable/extra-files/emptymain.d > ${output_file}
-rm -f emptymain.json || true
+$DMD -c -od=${dir} -Xi=compilerInfo compilable/extra-files/emptymain.d
+rm -f emptymain.json


Not sure why || true was added here. rm -f always exits with 0 and also this tests doesn't check the generated output.

Cause I didn't know rm -f always returned 0 :)

I remember I was having troubles trying to check the generated output, I can't remember why though...

wilzbach · 2018-03-13T15:17:50Z

test/compilable/test18367.sh


 # dmd should not segfault on -X with libraries, but no source files
 out=$("$DMD" -conf= -X foo.a 2>&1)
-[ $? -eq 1 ] || exit 1


That's exactly what -e is for. set -euo pipefail should probably be set by default, but that's a task for another PR.

wilzbach · 2018-03-13T15:19:37Z

test/compilable/issue17167.sh


 rm -rf "${dir:?}"/"$TEST_NAME"
-
-echo Success


We no longer need to write sth. to the output file to create it as it's now created by default and only removed in the error case. Of course, that behavior could be easily changed in the sh_do_test script, but the point is that this is superfluous, was often forgotten in the past and should be handled by the wrapper.

wilzbach · 2018-03-13T15:20:39Z

test/runnable/test16096.sh

-        cat "${output_file}" 1>&2
-    fi
-    rm -f "${output_file}"
-}


No need for custom rolled error handling anymore. This is the default behavior.

Depending on how the makefile is written, .DELETE_ON_ERROR might be required.

Oh this is now mostly handled by the sh_do_test wrapper.
Also removing the output file if an error happened isn't the default behavior because if

no input changed, then the output will still be the same

it always the developer to inspect the output file manually

wilzbach · 2018-03-13T15:21:39Z

test/runnable/test2.sh

-        cat ${output_file}
-        rm -f ${output_file}
-        exit 1
-    fi


No need for custom error handling anymore.

marler8997

Definitely an improvement :)

wilzbach · 2018-03-18T11:37:50Z

Pinging @CyberShadow as you seem to be the only one being able to approve this.

CyberShadow · 2018-03-18T15:13:46Z

Pinging @CyberShadow as you seem to be the only one being able to approve this.

What do you mean? Any committer can approve it.

wilzbach · 2018-03-19T22:51:33Z

What do you mean? Any committer can approve it.

Yes, but there aren't so many who feel comfortable with Bash - you and Martin are the only one who I know and Martin is traditionally very busy.
Sorry for the poor wording.

CyberShadow

Thanks! Looks good overall.

CyberShadow · 2018-03-19T23:34:34Z

test/runnable/depsprot.sh

 name=`basename $0 .sh`
 dir=${RESULTS_DIR}/runnable
 dmddir=${RESULTS_DIR}${SEP}runnable
-output_file=${dir}/${name}.sh.out


Looks like this script wanted to print its output only on failure? Is its output overly verbose, or something?

All output is already printed on a failure, so this is just redundant.

CyberShadow · 2018-03-19T23:35:32Z

test/compilable/crlf.sh


 # Test CRLF and mixed line ending handling in D lexer.

+set -e


👍

I'd go for the whole -euo pipefail trio. Variables that might be unset should be "declared" at the top of the file, i.e. replace unset variables with empty ones.

Yes, my plane is to enable the entire -euo pipefail trio by default for all bash scripts in sh_do_test.sh.
This is just an intermediate PR that should lay the ground work for this.
I just added -e here as it's an almost hard-coded reaction whenever I see || exit 1

CyberShadow · 2018-03-19T23:36:45Z

test/runnable/gdb15729.sh

 dir=${RESULTS_DIR}${SEP}runnable
-output_file=${dir}/gdb15729.sh.out
+
+set -e


Flags should probably come right after the shebang, since -u could be added later, and those variable definitions would benefit from -u's checks.

Done.
FYI: My plan is to add set -eu and maybe even set -o pipefail by default for ALL bash scripts as that would be a sane default.

CyberShadow · 2018-03-19T23:38:55Z

test/runnable/test16096.sh

-        cat "${output_file}" 1>&2
-    fi
-    rm -f "${output_file}"
-}


Depending on how the makefile is written, .DELETE_ON_ERROR might be required.

CyberShadow · 2018-03-19T23:46:33Z

test/sh_do_test.sh

+
+# redirect stdout + stderr to the output file + keep a reference to the std{out,err} streams for later
+exec 40>&1
+exec 41>&2


bash has a syntax to assign a free file descriptor and store it in a variable (exec {name_of_fd_var}>&1), which should guarantee that there will not be collisions, but the feature is "relatively" new and not sure if it's available on all test systems.

Nice! According to this SO answer it's available since available since bash 4.1+ (2009-12-31), so I just gave it a try.
Who knows, maybe it works on all systems

CyberShadow · 2018-03-19T23:47:20Z

test/sh_do_test.sh

+    if [ $1 -ne 0 ]; then
+        echo ==============================
+        echo Test ${script_name} failed. The logged output:
+        cat ${output_file}


General note, would be nice to see a lot more quoting. This line (and many many others) will fail if the variable contains spaces or some other special characters. ShellCheck helps greatly in identifying such moments.

FYI, I think if tests contained spaces, there's alot of places that would fail. But I suppose that's not a good reason to perpetuate bad practice.

CyberShadow · 2018-03-19T23:48:09Z

test/sh_do_test.sh

+    exec 2>&41
+
+    if [ $1 -ne 0 ]; then
+        echo ==============================


General note, printf > echo in scripts.

wilzbach commented Mar 13, 2018

View reviewed changes

wilzbach force-pushed the bash-output_file branch 2 times, most recently from 1ed652e to 786e3de Compare March 13, 2018 15:30

marler8997 approved these changes Mar 13, 2018

View reviewed changes

wilzbach force-pushed the bash-output_file branch from 786e3de to 3e02d2b Compare March 15, 2018 14:23

wilzbach mentioned this pull request Mar 15, 2018

Add and use RESULTS_TEST_DIR + OUTPUT_BASE + EXTRA_FILES in the bash scripts of DMD's testsuite #8038

Merged

wilzbach force-pushed the bash-output_file branch 2 times, most recently from ecbd7fc to d9a7f8e Compare March 18, 2018 11:36

wilzbach added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Mar 18, 2018

CyberShadow approved these changes Mar 19, 2018

View reviewed changes

JinShil added the Merge:auto-merge label Mar 21, 2018

wilzbach force-pushed the bash-output_file branch from d9a7f8e to 235b297 Compare March 21, 2018 23:54

dlang-bot removed the Merge:auto-merge label Mar 21, 2018

wilzbach force-pushed the bash-output_file branch 3 times, most recently from dc811a5 to 1de3cd5 Compare March 22, 2018 16:58

sh_do_test: verbose trace + start to remove inferred output_file

6052624

wilzbach force-pushed the bash-output_file branch from 1de3cd5 to 6052624 Compare March 22, 2018 16:58

wilzbach added the Merge:auto-merge label Mar 22, 2018

dlang-bot merged commit ad598ec into dlang:master Mar 22, 2018

wilzbach deleted the bash-output_file branch March 22, 2018 20:37


		# Test CRLF and mixed line ending handling in D lexer.

		set -e

Uh oh!

Conversation

wilzbach commented Mar 13, 2018

Uh oh!

dlang-bot commented Mar 13, 2018

Bugzilla references

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marler8997 left a comment

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Mar 18, 2018

Uh oh!

CyberShadow commented Mar 18, 2018

Uh oh!

wilzbach commented Mar 19, 2018

Uh oh!

CyberShadow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

wilzbach Mar 13, 2018 •

edited

Loading

wilzbach Mar 21, 2018 •

edited

Loading

wilzbach Mar 13, 2018 •

edited

Loading

wilzbach Mar 21, 2018 •

edited

Loading

wilzbach Mar 22, 2018 •

edited

Loading