Allow single test to be specified as an argument to run-nio-alloc-counter-tests.sh. #1214
Conversation
Lukasa
added
the
patch-version-bump-only
| @@ -18,25 +18,24 @@ here="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" | |||
|
|
|||
| tmp_dir="/tmp" | |||
|
|
|||
| function die() { | |||
There was a problem hiding this comment.
could we leave this in so we get better error messages?
There was a problem hiding this comment.
Yeah for sure. The reason I removed it is that it didn't seem like it was actually providing better error messages. For example, if you specify an unknown flag, you get the following output:
$ ./run-nio-alloc-counter-tests.sh -d
./run-nio-alloc-counter-tests.sh: illegal option -- d
ERROR: unknown option ?
The ? in this case is $opt, which is always "?" because it doesn't know what the option is. All of the useful information is contained in the first line, which is the error message from getopts.
Removing the only call to die (and then the die function itself because it's no longer used) gives the following output, which seemed a bit cleaner and less confusing:
$ ./run-nio-alloc-counter-tests.sh -d
./run-nio-alloc-counter-tests.sh: illegal option -- d
Happy to revert back to the previous code if you prefer though!
There was a problem hiding this comment.
oh I see, getopts actually prints an error itself. I'm happy with that then.
| while getopts "t:" opt; do | ||
| case "$opt" in | ||
| t) | ||
| tmp_dir="$OPTARG" | ||
| ;; | ||
| \?) |
There was a problem hiding this comment.
My thinking was most of the examples of getopts I've seen use * to handle the unknown case, as it's idiomatic with the default branch in a case block in general.
With \?, you have to remember that getopts will set the flag variable to ? if it doesn't recognise a flag, so it's the unknown case you're handling with this branch. You also have to remember that you don't need a separate default branch for the case, because the variable will always be ? if a flag is unknown.
Compared to a list of flags and then a default * case, that seemed less clear and harder to remember at first glance.
I don't feel strongly about it though, happy to revert.
| ;; | ||
| esac | ||
| done | ||
|
|
||
| shift $((OPTIND-1)) | ||
|
|
||
| test_pattern="${1:-test_*.swift}" |
There was a problem hiding this comment.
I think a better way to do this (which also doesn't break on newlines) is to start with
tests_to_run=( "$here"/test_*.swift )
and if we only run one test, then we can override this with all the arguments that we were given:
if [[ $OPTIND != $# ]]; then # put the right maths to figure out 'no arguments'
tests_to_run=$@ # override the tests to just contain the ones specified on the command line
fi
then, below we can use ...
There was a problem hiding this comment.
Yeah great suggestion. I only handled specifying a single test argument, as that was the use case I kept running into. But there's no reason not to allow multiple tests to be specified, and this is a good implementation for that.
I'll update!
There was a problem hiding this comment.
Ah I think I might have misunderstood the intention here. Your suggestion will still only run one test right? It will just treat all of the remaining arguments as a single test argument?
If I change "${tests_to_run[@]}" to ${tests_to_run[@]} below, then it will allow running multiple tests. Was that what you were picturing? (This won't handle whitespace in filenames however.)
| "$here/../../allocation-counter-tests-framework/run-allocation-counter.sh" \ | ||
| -p "$here/../../.." \ | ||
| -m NIO -m NIOHTTP1 -m NIOWebSocket \ | ||
| -s "$here/shared.swift" \ | ||
| -t "$tmp_dir" \ | ||
| "$here"/test_*.swift | ||
| "$here"/$test_pattern |
…nter-tests.sh. Motivation: run-nio-alloc-counter-tests.sh currently runs all tests. When writing or debugging a test, you commonly want to run a single test. At the moment the common practice is to edit the script to hardcode a test to run, which is annoying and error-prone. Modifications: Add an optional argument to the script to specify the test to run. Result: $ ./run-nio-alloc-counter-tests.sh runs all tests. $ ./run-nio-alloc-counter-tests.sh test_decode_1000_ws_frames.swift runs only test_decode_1000_ws_frames.swift.
|
@swift-nio-bot test this please |
Motivation:
run-nio-alloc-counter-tests.shcurrently runs all tests. When writing ordebugging a test, you commonly want to run a single test. At the moment the
common practice is to edit the script to hardcode a test to run, which is
annoying and error-prone.
Modifications:
Add an optional argument to the script to specify the test to run.
Result:
$ ./run-nio-alloc-counter-tests.shruns all tests.
$ ./run-nio-alloc-counter-tests.sh test_decode_1000_ws_frames.swiftruns only test_decode_1000_ws_frames.swift.