diff mbox series

test: allow taking test names from commandline

Message ID 20210127174255.1671738-1-bruce.richardson@intel.com (mailing list archive)
State Accepted, archived
Delegated to: David Marchand
Headers
Series test: allow taking test names from commandline |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-abi-testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-testing fail Testing issues

Commit Message

Bruce Richardson Jan. 27, 2021, 5:42 p.m. UTC 
  While having the ability to run a test based off the DPDK_TEST environment
variable is useful, it's often easier to specify the test name as a
commandline parameter to a test binary. This also allows the test runs to
be saved as part of the shell cmdline history.

This patch adds support for checking all parameters after the EAL ones, and
running all valid autotests requested - either from DPDK_TEST or on the
commandline. This also allows multiple tests to be run in a single
automated session, which is useful for working with components which have
multiple test suites.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/commands.c | 11 +++++++++++
 app/test/test.c     | 40 +++++++++++++++++++++++++++++++++-------
 app/test/test.h     |  1 +
 3 files changed, 45 insertions(+), 7 deletions(-)

--
2.27.0

Comments

Aaron Conole Jan. 29, 2021, 8:22 p.m. UTC | #1 
Bruce Richardson <bruce.richardson@intel.com> writes:

> While having the ability to run a test based off the DPDK_TEST environment
> variable is useful, it's often easier to specify the test name as a
> commandline parameter to a test binary. This also allows the test runs to
> be saved as part of the shell cmdline history.
>
> This patch adds support for checking all parameters after the EAL ones, and
> running all valid autotests requested - either from DPDK_TEST or on the
> commandline. This also allows multiple tests to be run in a single
> automated session, which is useful for working with components which have
> multiple test suites.
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---

LGTM, and I think it can be useful for running specific tests when
developers make changes.

Acked-by: Aaron Conole <aconole@redhat.com>
David Marchand April 9, 2021, 1:27 p.m. UTC | #2 
On Wed, Jan 27, 2021 at 6:43 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> While having the ability to run a test based off the DPDK_TEST environment
> variable is useful, it's often easier to specify the test name as a
> commandline parameter to a test binary. This also allows the test runs to
> be saved as part of the shell cmdline history.

I don't get the argument about history:

$ history |grep DPDK_TEST
10615  2021-03-24 10:42:11 ninja-build -C build -j4 &&
DPDK_TEST=logs_autotest ./build/app/test/dpdk-test --no-huge -m 512
--log-level=lib.eal:debug
10636  2021-03-24 10:51:09 ninja-build -C build -j4 &&
DPDK_TEST=logs_autotest ./build/app/test/dpdk-test --no-huge -m 512
--log-level=lib.eal:debug
10653  2021-03-24 11:17:01 ninja-build -C build -j4 &&
DPDK_TEST=kvargs_autotest ./build/app/test/dpdk-test --no-huge -m 512
--log-level=lib.eal:debug
10794  2021-03-25 18:37:48 history |grep DPDK_TEST


>
> This patch adds support for checking all parameters after the EAL ones, and
> running all valid autotests requested - either from DPDK_TEST or on the
> commandline. This also allows multiple tests to be run in a single
> automated session, which is useful for working with components which have
> multiple test suites.

The same could be achieved splitting DPDK_TEST content with spaces,
since test names don't contain one.
Bruce Richardson April 9, 2021, 1:41 p.m. UTC | #3 
On Fri, Apr 09, 2021 at 03:27:17PM +0200, David Marchand wrote:
> On Wed, Jan 27, 2021 at 6:43 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > While having the ability to run a test based off the DPDK_TEST environment
> > variable is useful, it's often easier to specify the test name as a
> > commandline parameter to a test binary. This also allows the test runs to
> > be saved as part of the shell cmdline history.
> 
> I don't get the argument about history:
> 
> $ history |grep DPDK_TEST
> 10615  2021-03-24 10:42:11 ninja-build -C build -j4 &&
> DPDK_TEST=logs_autotest ./build/app/test/dpdk-test --no-huge -m 512
> --log-level=lib.eal:debug
> 10636  2021-03-24 10:51:09 ninja-build -C build -j4 &&
> DPDK_TEST=logs_autotest ./build/app/test/dpdk-test --no-huge -m 512
> --log-level=lib.eal:debug
> 10653  2021-03-24 11:17:01 ninja-build -C build -j4 &&
> DPDK_TEST=kvargs_autotest ./build/app/test/dpdk-test --no-huge -m 512
> --log-level=lib.eal:debug
> 10794  2021-03-25 18:37:48 history |grep DPDK_TEST
> 

Sure, if you always specify the test name explicitly for each command,
rather than running the one test multiple times having set it separately in
the environment.
Overall, though I take the point that from a history saving point of view
it's a minor saving.

> 
> >
> > This patch adds support for checking all parameters after the EAL ones, and
> > running all valid autotests requested - either from DPDK_TEST or on the
> > commandline. This also allows multiple tests to be run in a single
> > automated session, which is useful for working with components which have
> > multiple test suites.
> 
> The same could be achieved splitting DPDK_TEST content with spaces,
> since test names don't contain one.
> 

Yep, that's a useful enhancement too, but I still thing it's better to just
have the list of tests appended to the test binary command rather than have
to be worrying about properly quoting a specific environment variable at
the start of each command.

/Bruce
Bruce Richardson April 13, 2021, 4:48 p.m. UTC | #4 
On Fri, Apr 09, 2021 at 02:41:11PM +0100, Bruce Richardson wrote:
> On Fri, Apr 09, 2021 at 03:27:17PM +0200, David Marchand wrote:
> > On Wed, Jan 27, 2021 at 6:43 PM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > >
> > > While having the ability to run a test based off the DPDK_TEST environment
> > > variable is useful, it's often easier to specify the test name as a
> > > commandline parameter to a test binary. This also allows the test runs to
> > > be saved as part of the shell cmdline history.
> > 
> > I don't get the argument about history:
> > 
> > $ history |grep DPDK_TEST
> > 10615  2021-03-24 10:42:11 ninja-build -C build -j4 &&
> > DPDK_TEST=logs_autotest ./build/app/test/dpdk-test --no-huge -m 512
> > --log-level=lib.eal:debug
> > 10636  2021-03-24 10:51:09 ninja-build -C build -j4 &&
> > DPDK_TEST=logs_autotest ./build/app/test/dpdk-test --no-huge -m 512
> > --log-level=lib.eal:debug
> > 10653  2021-03-24 11:17:01 ninja-build -C build -j4 &&
> > DPDK_TEST=kvargs_autotest ./build/app/test/dpdk-test --no-huge -m 512
> > --log-level=lib.eal:debug
> > 10794  2021-03-25 18:37:48 history |grep DPDK_TEST
> > 
> 
> Sure, if you always specify the test name explicitly for each command,
> rather than running the one test multiple times having set it separately in
> the environment.
> Overall, though I take the point that from a history saving point of view
> it's a minor saving.
> 
> > 
> > >
> > > This patch adds support for checking all parameters after the EAL ones, and
> > > running all valid autotests requested - either from DPDK_TEST or on the
> > > commandline. This also allows multiple tests to be run in a single
> > > automated session, which is useful for working with components which have
> > > multiple test suites.
> > 
> > The same could be achieved splitting DPDK_TEST content with spaces,
> > since test names don't contain one.
> > 
> 
> Yep, that's a useful enhancement too, but I still thing it's better to just
> have the list of tests appended to the test binary command rather than have
> to be worrying about properly quoting a specific environment variable at
> the start of each command.
> 
Ping on this.

Other instances where using cmdline is preferred over environment

* Calling tests using sudo.
* Calling tests as an execute action when doing a git rebase

Yes, again in both cases, other workarounds are generally available (e.g.
sudo -E, and exporting to environemtn before rebase), but also generally
less convenient.

/Bruce
David Marchand April 14, 2021, 6:12 a.m. UTC | #5 
On Tue, Apr 13, 2021 at 6:49 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Fri, Apr 09, 2021 at 02:41:11PM +0100, Bruce Richardson wrote:
> > On Fri, Apr 09, 2021 at 03:27:17PM +0200, David Marchand wrote:
> > > On Wed, Jan 27, 2021 at 6:43 PM Bruce Richardson
> > > <bruce.richardson@intel.com> wrote:
> > > >
> > > > While having the ability to run a test based off the DPDK_TEST environment
> > > > variable is useful, it's often easier to specify the test name as a
> > > > commandline parameter to a test binary. This also allows the test runs to
> > > > be saved as part of the shell cmdline history.
> > >
> > > I don't get the argument about history:
> > >
> > > $ history |grep DPDK_TEST
> > > 10615  2021-03-24 10:42:11 ninja-build -C build -j4 &&
> > > DPDK_TEST=logs_autotest ./build/app/test/dpdk-test --no-huge -m 512
> > > --log-level=lib.eal:debug
> > > 10636  2021-03-24 10:51:09 ninja-build -C build -j4 &&
> > > DPDK_TEST=logs_autotest ./build/app/test/dpdk-test --no-huge -m 512
> > > --log-level=lib.eal:debug
> > > 10653  2021-03-24 11:17:01 ninja-build -C build -j4 &&
> > > DPDK_TEST=kvargs_autotest ./build/app/test/dpdk-test --no-huge -m 512
> > > --log-level=lib.eal:debug
> > > 10794  2021-03-25 18:37:48 history |grep DPDK_TEST
> > >
> >
> > Sure, if you always specify the test name explicitly for each command,
> > rather than running the one test multiple times having set it separately in
> > the environment.
> > Overall, though I take the point that from a history saving point of view
> > it's a minor saving.
> >
> > >
> > > >
> > > > This patch adds support for checking all parameters after the EAL ones, and
> > > > running all valid autotests requested - either from DPDK_TEST or on the
> > > > commandline. This also allows multiple tests to be run in a single
> > > > automated session, which is useful for working with components which have
> > > > multiple test suites.
> > >
> > > The same could be achieved splitting DPDK_TEST content with spaces,
> > > since test names don't contain one.
> > >
> >
> > Yep, that's a useful enhancement too, but I still thing it's better to just
> > have the list of tests appended to the test binary command rather than have
> > to be worrying about properly quoting a specific environment variable at
> > the start of each command.
> >
> Ping on this.

I have been using the DPDK_TEST= method for quite some time.
But I don't think I ever had a need to run multiple tests since I
usually track regressions or hard to reproduce failures.
When developing a new test, idem, I used DPDK_TEST=.


>
> Other instances where using cmdline is preferred over environment
>
> * Calling tests using sudo.

I guess calling a shell with the same command would work, something like:
$ sudo sh -c 'DPDK_TEST=debug_autotest ..../build/app/test/dpdk-test
--no-huge -m 512'

> * Calling tests as an execute action when doing a git rebase

I am pretty sure I used DPDK_TEST= in rebase execute actions in the past too.
This probably works too:
$ git rebase -i HEAD^ -x 'DPDK_TEST=debug_autotest
build/app/test/dpdk-test --no-huge -m 512'


>
> Yes, again in both cases, other workarounds are generally available (e.g.
> sudo -E, and exporting to environemtn before rebase), but also generally
> less convenient.

You find it less convenient when you want to run multiple tests.
I don't have that use case but I don't mind taking it.
Can you simply adjust the commitlog, or propose a new wording I can
take when applying?


Thanks.
Bruce Richardson April 14, 2021, 10:02 a.m. UTC | #6 
On Wed, Apr 14, 2021 at 08:12:50AM +0200, David Marchand wrote:
> On Tue, Apr 13, 2021 at 6:49 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Fri, Apr 09, 2021 at 02:41:11PM +0100, Bruce Richardson wrote:
> > > On Fri, Apr 09, 2021 at 03:27:17PM +0200, David Marchand wrote:
> > > > On Wed, Jan 27, 2021 at 6:43 PM Bruce Richardson
> > > > <bruce.richardson@intel.com> wrote:
> > > > >
> > > > > While having the ability to run a test based off the DPDK_TEST environment
> > > > > variable is useful, it's often easier to specify the test name as a
> > > > > commandline parameter to a test binary. This also allows the test runs to
> > > > > be saved as part of the shell cmdline history.
> > > >
> > > > I don't get the argument about history:
> > > >
> > > > $ history |grep DPDK_TEST
> > > > 10615  2021-03-24 10:42:11 ninja-build -C build -j4 &&
> > > > DPDK_TEST=logs_autotest ./build/app/test/dpdk-test --no-huge -m 512
> > > > --log-level=lib.eal:debug
> > > > 10636  2021-03-24 10:51:09 ninja-build -C build -j4 &&
> > > > DPDK_TEST=logs_autotest ./build/app/test/dpdk-test --no-huge -m 512
> > > > --log-level=lib.eal:debug
> > > > 10653  2021-03-24 11:17:01 ninja-build -C build -j4 &&
> > > > DPDK_TEST=kvargs_autotest ./build/app/test/dpdk-test --no-huge -m 512
> > > > --log-level=lib.eal:debug
> > > > 10794  2021-03-25 18:37:48 history |grep DPDK_TEST
> > > >
> > >
> > > Sure, if you always specify the test name explicitly for each command,
> > > rather than running the one test multiple times having set it separately in
> > > the environment.
> > > Overall, though I take the point that from a history saving point of view
> > > it's a minor saving.
> > >
> > > >
> > > > >
> > > > > This patch adds support for checking all parameters after the EAL ones, and
> > > > > running all valid autotests requested - either from DPDK_TEST or on the
> > > > > commandline. This also allows multiple tests to be run in a single
> > > > > automated session, which is useful for working with components which have
> > > > > multiple test suites.
> > > >
> > > > The same could be achieved splitting DPDK_TEST content with spaces,
> > > > since test names don't contain one.
> > > >
> > >
> > > Yep, that's a useful enhancement too, but I still thing it's better to just
> > > have the list of tests appended to the test binary command rather than have
> > > to be worrying about properly quoting a specific environment variable at
> > > the start of each command.
> > >
> > Ping on this.
> 
> I have been using the DPDK_TEST= method for quite some time.
> But I don't think I ever had a need to run multiple tests since I
> usually track regressions or hard to reproduce failures.
> When developing a new test, idem, I used DPDK_TEST=.
> 
> 
> >
> > Other instances where using cmdline is preferred over environment
> >
> > * Calling tests using sudo.
> 
> I guess calling a shell with the same command would work, something like:
> $ sudo sh -c 'DPDK_TEST=debug_autotest ..../build/app/test/dpdk-test
> --no-huge -m 512'

Not exactly the simplest command, compared to just
 "sudo /path/to/dpdk-test --no-huge -m512 -- debug_autotest"
Using sudo -E is what I tend to prefer in this case. :-)

> 
> > * Calling tests as an execute action when doing a git rebase
> 
> I am pretty sure I used DPDK_TEST= in rebase execute actions in the past too.
> This probably works too:
> $ git rebase -i HEAD^ -x 'DPDK_TEST=debug_autotest
> build/app/test/dpdk-test --no-huge -m 512'
> 
> 
> >
> > Yes, again in both cases, other workarounds are generally available (e.g.
> > sudo -E, and exporting to environemtn before rebase), but also generally
> > less convenient.
> 
> You find it less convenient when you want to run multiple tests.
> I don't have that use case but I don't mind taking it.
> Can you simply adjust the commitlog, or propose a new wording I can
> take when applying?
> 
Would just replacing the first paragraph as below be suitable? Since it's
only a single sentence, the second paragraph can also be merged straight in
with it to leave a 1-paragraph commit log message.

"While having the ability to run a test based off the DPDK_TEST
environment variable is useful, it's sometimes more convenient to specify
the test name as a commandline parameter to a test binary."

/Bruce
David Marchand April 14, 2021, 1:25 p.m. UTC | #7 
On Fri, Jan 29, 2021 at 9:22 PM Aaron Conole <aconole@redhat.com> wrote:
>
> Bruce Richardson <bruce.richardson@intel.com> writes:
>
> > While having the ability to run a test based off the DPDK_TEST environment
> > variable is useful, it's often easier to specify the test name as a
> > commandline parameter to a test binary. This also allows the test runs to
> > be saved as part of the shell cmdline history.
> >
> > This patch adds support for checking all parameters after the EAL ones, and
> > running all valid autotests requested - either from DPDK_TEST or on the
> > commandline. This also allows multiple tests to be run in a single
> > automated session, which is useful for working with components which have
> > multiple test suites.
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> Acked-by: Aaron Conole <aconole@redhat.com>

Applied with updated commitlog, thanks.
diff mbox series

Patch

diff --git a/app/test/commands.c b/app/test/commands.c
index d48dd513d7..76f6ee5d23 100644
--- a/app/test/commands.c
+++ b/app/test/commands.c
@@ -381,3 +381,14 @@  int commands_init(void)
 	cmd_autotest_autotest.string_data.str = commands;
 	return 0;
 }
+
+int command_valid(const char *cmd)
+{
+	struct test_command *t;
+
+	TAILQ_FOREACH(t, &commands_list, next) {
+		if (strcmp(t->command, cmd) == 0)
+			return 1;
+	}
+	return 0;
+}
diff --git a/app/test/test.c b/app/test/test.c
index 624dd48042..b4ef2bf22b 100644
--- a/app/test/test.c
+++ b/app/test/test.c
@@ -93,6 +93,9 @@  main(int argc, char **argv)
 {
 #ifdef RTE_LIB_CMDLINE
 	struct cmdline *cl;
+	char *tests[argc]; /* store an array of tests to run */
+	int test_count = 0;
+	int i;
 #endif
 	char *extra_args;
 	int ret;
@@ -147,6 +150,7 @@  main(int argc, char **argv)
 	}

 	argv += ret;
+	argc -= ret;

 	prgname = argv[0];

@@ -165,7 +169,25 @@  main(int argc, char **argv)

 #ifdef RTE_LIB_CMDLINE
 	char *dpdk_test = getenv("DPDK_TEST");
-	if (dpdk_test && strlen(dpdk_test)) {
+
+	if (dpdk_test && strlen(dpdk_test) == 0)
+		dpdk_test = NULL;
+
+	if (dpdk_test && !command_valid(dpdk_test)) {
+		RTE_LOG(WARNING, APP, "Invalid DPDK_TEST value '%s'\n", dpdk_test);
+		dpdk_test = NULL;
+	}
+
+	if (dpdk_test)
+		tests[test_count++] = dpdk_test;
+	for (i = 1; i < argc; i++) {
+		if (!command_valid(argv[i]))
+			RTE_LOG(WARNING, APP, "Invalid test requested: '%s'\n", argv[i]);
+		else
+			tests[test_count++] = argv[i];
+	}
+
+	if (test_count > 0) {
 		char buf[1024];

 		cl = cmdline_new(main_ctx, "RTE>>", 0, 1);
@@ -174,13 +196,17 @@  main(int argc, char **argv)
 			goto out;
 		}

-		snprintf(buf, sizeof(buf), "%s\n", dpdk_test);
-		if (cmdline_in(cl, buf, strlen(buf)) < 0) {
-			printf("error on cmdline input\n");
+		for (i = 0; i < test_count; i++) {
+			snprintf(buf, sizeof(buf), "%s\n", tests[i]);
+			if (cmdline_in(cl, buf, strlen(buf)) < 0) {
+				printf("error on cmdline input\n");

-			ret = -1;
-		} else {
-			ret = last_test_result;
+				ret = -1;
+			} else
+				ret = last_test_result;
+
+			if (ret != 0)
+				break;
 		}
 		cmdline_free(cl);
 		goto out;
diff --git a/app/test/test.h b/app/test/test.h
index b07f6c1ef0..cd047eb26c 100644
--- a/app/test/test.h
+++ b/app/test/test.h
@@ -152,6 +152,7 @@  extern int last_test_result;
 extern const char *prgname;

 int commands_init(void);
+int command_valid(const char *cmd);

 int test_mp_secondary(void);
 int test_timer_secondary(void);