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.

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

runm3uatest: Avoid having to use a ~/.guile script #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

laf0rge
Copy link

@laf0rge laf0rge commented Dec 15, 2021

Rather, we can specify the path in which the m3ua-testtool files are
located using the '-d ...' command line argument of runm3uatest

Rather, we can specify the path in which the m3ua-testtool files are
located using the '-d ...' command line argument of runm3uatest
@@ -26,7 +26,7 @@
;;; $Id: dotguile,v 1.1 2012/08/26 21:06:27 tuexen Exp $

;;; Change the following line to reflect where the files are located.
(define dir "/Users/tuexen/Documents/m3ua-testtool/")
(define dir "")
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to keep the directory name. On the one hand side it shows what to insert, on the other handside it breaks very early if you haven't changed it to an appropriate way.

Copy link
Author

Choose a reason for hiding this comment

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

I find it somewhat strange and uncommon that the personal home directory of the developer is hard-coded in some files that are distributed with a project. That's why I was suggesting to move to a "all relative paths' setup where no installation is required, no files to the home directory need to be deployed, etc.

If the home directory must be present in this file, then there should be some template / pattern substitution mechanism that uses the actual "$HOME" at the time of installation.

@@ -16,6 +16,6 @@ testcases='m3ua-asp-aspsm-v-002

for testcase in $testcases
do
runm3uatest -t $timeout $testcase 2> /dev/null
./runm3uatest -d . -t $timeout $testcase 2> /dev/null
Copy link
Contributor

Choose a reason for hiding this comment

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

This requires the binary to be in the directory where the script is. I assume that this is installed, which is also the case when running the tests manually.

Copy link
Author

Choose a reason for hiding this comment

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

requiring installation of a helper script to some system PATH seems like a relatively odd choice for a test tool. 99.9% of the FOSS software I know of can be executed from within the checked out git repo / build directory, without requiring system-wide installation. I see that a s a strong benefit, avoiding to clutter my system usr/local/bin etc. with tons of cruft over time.

You're of course free to reject it. With the python-based altrnative executor of the other patch we at osmocom don't even use runm3uatest anymore. It was just something developed at an earlier stage trying to make the nplab-tests run for the first time some 4 years ago.

@@ -0,0 +1 @@
dotguile
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why .guile is showing up here. This file does not exists in this dir and should not be here.

@@ -16,6 +16,6 @@ testcases='m3ua-sgp-aspsm-v-003

for testcase in $testcases
do
runm3uatest -t $timeout $testcase 2> /dev/null
./runm3uatest -d . -t $timeout $testcase 2> /dev/null
Copy link
Contributor

Choose a reason for hiding this comment

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

See above...


timeout = TIMEOUT;

while ((c = getopt(argc, argv, "t:")) != -1) {
while ((c = getopt(argc, argv, "t:d:")) != -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to keep the options in alphabetic order.

@@ -83,24 +84,27 @@ main(int argc, char *argv[]) {
case 't':
timeout = (unsigned int)atoi(optarg);
break;
case 'd':
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep the options in alphabetic order.

@osmocom-gerrit osmocom-gerrit deleted the laforge/avoid-dotguile branch July 15, 2022 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants