Move common BASH test logic to wrapper by marler8997 · Pull Request #7928 · dlang/dmd

marler8997 · 2018-02-20T22:24:18Z

This PR creates a wrapper to hold common logic/settings for all BASH tests. Currently, this wrapper will pipe stdout/stderr to the test output file, and when a failure occurs, will dump the output file with a message saying the test failed (just like d_do_test does with .d tests).

It also exports common variables to the test script, currently

${TEST_DIR} (i.e. compilable/runnable)
${TEST_NAME} being the base name of the test.

TODO:

Maybe set -euo pipefail and/or set -x in wrapper?

dlang-bot · 2018-02-20T22:24:19Z

Thanks for your pull request and interest in making D better, @marler8997! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

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-02-20T22:28:46Z

test/bash_test_wrapper.sh

+    # duplicate d_do_test output
+    echo >> ${output_file}
+    echo ============================== >> ${output_file}
+    echo Test failed >> ${output_file}


It's good to state here which test failed as the tests might run in high parallelism.

wilzbach

I'm strongly voting for this. Redefining the same variables in the bash scripts has become quite a pain.
Thanks a lot @marler8997 for pushing this!

I even have a few idea to make this more useful (a few more default exported variables, a default debug trap, default set -eu o pipefail, ...), but I'm more than happy to do this in a quick follow-up.

CC @MartinNowak @CyberShadow

wilzbach · 2018-02-23T00:15:37Z

test/compilable/crlf.sh

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

-name=`basename $0 .sh`
 dir=${RESULTS_DIR}/compilable


this is no longer used?

yeah, replaced with ${TEST_NAME}

wilzbach · 2018-02-23T00:15:56Z

test/compilable/crlf.sh

-name=`basename $0 .sh`
 dir=${RESULTS_DIR}/compilable
-fn=${dir}/${name}.d
+fn=${TEST_DIR}/${TEST_NAME}.d


In a next step TEST_FILE could be exported as well.

wilzbach · 2018-02-23T00:17:35Z

test/compilable/ddoc9764.sh

-name=`basename $0 .sh`
-dir=${RESULTS_DIR}/compilable
-output_file=${dir}/${name}.html
+output_file=${RESULTS_DIR}/${TEST_DIR}/${TEST_NAME}.html


This seems to be quite frequent enough.

export OUTPUT_FILE="${RESULTS_DIR}/${TEST_DIR}/${TEST_NAME}"

could be part of a next step.

this one is common enough, I'll add it to this PR

actually, wait, this is specific to this test only...it has the .html extension

Yes, that's why export OUTPUT_FILE="${RESULTS_DIR}/${TEST_DIR}/${TEST_NAME}" which allows the user to pick the extension, but yeah, let's think about this in the next step.

Yeah, I wasn't sure the best variables to expose...there's combinations of the source dir vs results dir and different extensions...we can figure it out in the next step like you said

wilzbach · 2018-02-23T00:20:42Z

test/sh_do_test.sh

+
+# TEST_DIR should be one of compilable, fail_compilation or runnable
+export TEST_DIR=$1
+export TEST_NAME=$2


There are a few more variables which I frequently need, but I think we can easily go step by step here.

wilzbach · 2018-02-23T00:21:17Z

test/sh_do_test.sh

@@ -0,0 +1,29 @@
+#!/usr/bin/env bash
+


set -u -o pipefail (doesn't have to be done in this PR)

wilzbach · 2018-02-23T00:24:36Z

test/sh_do_test.sh

+output_file=${RESULTS_DIR}/${script_name}.out
+rm -f ${output_file}
+
+./${script_name} > ${output_file}  2>&1


A future extension could use source here.
Advantages:

use a trap, to show the error of the failure

activate set -ue -o pipefail by default

activate set +x by default, redirect the debug log into a XYZ.debug file and only show it when there's a failure.

wilzbach · 2018-02-23T00:33:01Z

Maybe set -euo pipefail and/or set -x in wrapper?

Yes, we should do this, but I think your PR is already a good start and makes a good base for future improvements like this one (or the other potential ones I have mentioned).

marler8997 · 2018-02-23T04:27:04Z

Ok yeah, we can merge as is and I'll let you enhance it (you're better at BASH anyway)

wilzbach · 2018-02-27T23:10:35Z

No one has objected to this change within one week, it's not critical DMD code, but just affects the test suite and we need to move on -> merging.

wilzbach reviewed Feb 20, 2018

View reviewed changes

marler8997 force-pushed the betterBashTests branch 3 times, most recently from ce1fae7 to d7e3b3e Compare February 21, 2018 01:54

marler8997 mentioned this pull request Feb 21, 2018

Use "" (empty argument) to indicate no file after -run #7927

Closed

marler8997 force-pushed the betterBashTests branch 3 times, most recently from 503596f to e7e8b69 Compare February 21, 2018 17:05

Created sh_do_test to wrap BASH tests with common logic/settings

a0d1f08

marler8997 force-pushed the betterBashTests branch from e7e8b69 to a0d1f08 Compare February 22, 2018 16:31

wilzbach approved these changes Feb 23, 2018

View reviewed changes

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

wilzbach merged commit 9936d85 into dlang:master Feb 27, 2018

marler8997 mentioned this pull request Feb 28, 2018

Print DFLAGS on -v to ease debugging #7865

Merged

marler8997 deleted the betterBashTests branch February 28, 2018 12:42

wilzbach mentioned this pull request Mar 13, 2018

sh_do_test: verbose trace + start to remove inferred output_file #8025

Merged

Uh oh!

Conversation

marler8997 commented Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Feb 20, 2018

Bugzilla references

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach 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!

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!

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 commented Feb 23, 2018

Uh oh!

marler8997 commented Feb 23, 2018

Uh oh!

wilzbach commented Feb 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

marler8997 commented Feb 20, 2018 •

edited

Loading