Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow single test to be specified as an argument to run-nio-alloc-counter-tests.sh. #1214

Merged
merged 3 commits into from Nov 7, 2019
Merged

Allow single test to be specified as an argument to run-nio-alloc-counter-tests.sh. #1214

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
64acaf9
Allow single test to be specified as an argument to run-nio-alloc-cou…
Nov 1, 2019
44c70c4
Update run-nio-alloc-counter-tests.sh
weissi Nov 6, 2019
9ecdd4f
Merge branch 'master' into script_param
weissi Nov 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 11 additions & 8 deletions IntegrationTests/tests_04_performance/test_01_resources/run-nio-alloc-counter-tests.sh
View file
Open in desktop
Expand Up @@ -18,25 +18,28 @@ here="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"

tmp_dir="/tmp"

function die() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we leave this in so we get better error messages?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I see, getopts actually prints an error itself. I'm happy with that then.

echo >&2 "ERROR: $*"
exit 1
}

while getopts "t:" opt; do
case "$opt" in
t)
tmp_dir="$OPTARG"
;;
\?)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change?

Copy link
Contributor Author

@marlimox marlimox Nov 6, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

die "unknown option $opt"
*)
exit 1
;;
esac
done

shift $((OPTIND-1))

tests_to_run=("$here"/test_*.swift)

if [[ $# -gt 0 ]]; then
tests_to_run=$@
fi

"$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
"${tests_to_run[@]}"