autotest: add autotest framework for utility unit tests #348

hanw · 2017-03-06T17:53:09Z

We don't have a framework for testing utility tools, such as compiler driver. Here is an attempt to add support for autotest framework.

@ChrisDodd Can you take a look and let me know if you are okay with the approach?

mihaibudiu · 2017-03-06T19:23:06Z

Makefile.am

@@ -40,6 +40,7 @@ noinst_LIBRARIES =             # Static libraries built
 noinst_PROGRAMS =              # Binaries built that are not installed
 BUILT_SOURCES =                # Generated source files
 TESTS =                        # Tests to execute
+EXTRA_DIST =                   # ...


Please document this.

mihaibudiu · 2017-03-06T19:28:48Z

tests/helloworld.at

+AT_SETUP([p4c])
+
+m4_define([P4C_CHECK], [
+AT_CHECK([p4c -t tofino min.p4], [1],


There is no p4c binary in the public repository.

I am working on a compiler driver called p4c which is analogous to clang or gcc. I will push the code for p4c before this PR goes in.

You should write a document describing what the compiler driver is supposed to do.
This code is really the infrastructure for building a family of compilers, one for each target. Since we don't know the needs of each target, I am not sure whether it's easy to create a driver common to all targets.

mihaibudiu · 2017-03-06T19:39:49Z

tests/p4ctest.h

+ *   ....
+ * }
+ *
+ * P4CTEST_REGISTER("my-test", my_test_main);


How about REGISTER taking just one argument? The string should be generated from the test name; perhaps you can always append _main as well.

mihaibudiu · 2017-03-06T19:41:53Z

tests/Makefile.am

@@ -0,0 +1,89 @@
+# Copyright 2013-present Barefoot Networks, Inc.


I don't really know how this autotest stuff works, and I am trying to avoid learning if possible.
It would be nice if the extensions/ folder was used for testing as well - so you can actually "merge" the tests here with whatever tests/ folders may exist in any extension.

added an example in backend/p4test/tests to show how to extend unit test with custom backend.

mihaibudiu · 2017-03-06T19:48:40Z

tests/p4ctest.cpp

+}
+
+static void
+add_command(struct p4c_cmdl_command* cmd)


I think that the style guide would require opening brace on the previous line.

mihaibudiu · 2017-03-06T19:49:27Z

tests/p4ctest.cpp

+#include "lib/error.h"
+#include "p4ctest.h"
+
+static std::vector<p4c_cmdl_command*> commands;


Why not package all of these in a class?

mihaibudiu · 2017-03-06T19:51:41Z

tests/p4ctest.cpp

+                    exit(1);
+                }
+                if (ferror(stderr)) {
+                    std::cerr << "write to stderr failed" << std::endl;


Is this expected to succeed? Probably it does not matter.

mihaibudiu · 2017-03-06T19:54:22Z

tests/p4ctest.h

+struct p4c_cmdl_command {
+    const char *name;
+    const char *usage;
+    int min_args;


I don't know how these are used.

hanw · 2017-03-07T21:59:56Z

@ChrisDodd @mbudiu-vmw This patch is ready for review.

mihaibudiu · 2017-03-08T00:48:04Z

tests/p4ctest.h

+/* Overview
+ * ========
+ *
+ * P4C tests directory contains many small test programs. One down side


I think that this comment should not describe the history of the program creation, it should just say what the program does. This information should be in the pull request, but not in a comment - it will become obsolete after a while.

mihaibudiu · 2017-03-08T00:48:29Z

tests/ir.at

@@ -0,0 +1,22 @@
+AT_BANNER([intermediate representation unit tests])


I hope that users writing tests won't have to learn this language...

m4 syntax takes some getting used to, particularly the automake dialect of it.

This still defines the same macro mulitple times with different names (just once in each file) -- might it make more sense to call it P4TEST_TEST_RUN or (something less verbose) and put it in the top level file (testsuite.at?) That way each sub .at file is just a list of tests.

ChrisDodd · 2017-03-08T00:29:18Z

configure.ac

 AM_INIT_AUTOMAKE([foreign subdir-objects 1.14])
-AM_SILENT_RULES([yes])
+AM_SILENT_RULES([no])
+AM_MISSING_PROG([AUTOM4TE], [autom4te])


Generally I much prefer silent rules as it makes it easier to see compiler warnings (not buried in masses of libtool invokation cruft). Using V=1 is usually adequate for debugging make problems.

ChrisDodd · 2017-03-08T00:51:06Z

tests/p4ctest.h

+ * all test programs into a single program, p4c-test. Regardless of
+ * the number of test programs, linking will be done only once to produce
+ * p4ctest.
+ *


How much time does this actually save? Each little program only take about .3 seconds to link on my machine, so it seems unlike to save more that a second or two.

removed comments.

ChrisDodd · 2017-03-08T00:55:53Z

tests/ir.at

+ AT_CHECK([p4ctest $2], [0], [stdout], [])
+ AT_CLEANUP]
+)
+TEST_IR_OPEQ([ir - transform ], [test-transform])


This seems like a lot of boilerplate -- why define a new TEST_XXX macro for each test that you then invoke once, particularly since they're all the same. Why not just a single macro invoked mulitple times (once for each test).

Removed /unittests, divided existing unit tests into: ir, frontend and util groups.

hanw · 2017-03-13T23:39:27Z

@ChrisDodd How about the following usage?

make check D=1 // for enabling -gdb debugging
make check J=8 // for enabling -j8 flag
make check N=8, 10-12 // for running test case 8, 10, 11 and 12.
make check V=1 // for enabling verbose mode.
make check K=bmv2 // for run all test cases for bmv2.

I can try to generate a simplified version of testsuite.log file that only contains information about the failed cases, and save the info in test-suite.log

I can print the command to rerun a test case as part of the log output, so you can click on the string and copy/paste to terminal.

Let me know if there are more options you would like to have.

ChrisDodd · 2017-03-13T23:49:20Z

For -j, you probably need a long name so it doesn't collide with other things; most people will just want to set it in their .bashrc file and always have it 'just work'. That's the way MAKEFLAGS=-j8 works, for the most part

V=1 is going to collide with the existing use of V for verbose builds

How do you run a particular test by name? How do you know what "test 47" is -- how do you find the source and intermediate files of the test so you can figure out what went wrong?

cc10512 · 2017-03-13T23:59:38Z

@hanw: isn't it just a matter of propagating $MAKEFLAGS to the make commands to the testsuite command? tests/testsuite --help has this gem:

  -j, --jobs[=N]
                 Allow N jobs at once; infinite jobs with no arg (default 1)

hanw · 2017-03-14T00:49:29Z

I am able to make these to work.
make check-local MAKEFLAGS=-j8
make check-local AM_MAKEFLAGS=-j8
AM_MAKEFLAGS=-j8 make check-local

but not these two.
MAKEFLAGS=-j8 make check-local
make check-local -j8

In the second case, MAKEFLAGS has a value of "-j --jobserver-fds=4,5", instead of what I set on command line (-j8). not sure where it comes from.

cc10512 · 2017-03-14T00:55:56Z

tests/Makefile.am

+	"$$@" || (test X'$(RECHECK)' = Xyes && "$$@" --recheck)
+
+check-bmv2:
+	make check-local TESTSUITEFLAGS='-k bmv2 $(filter-out $@,$(MAKECMDGOALS))'


have you tried passing $(MAKEFLAGS) to this command? That is:

check-bmv2: make check-local TESTSUITEFLAGS='-k bmv2 $(filter-out $@,$(MAKECMDGOALS))' $(MAKEFLAGS)

This turns out to be a known issue in make 4.1, where the value N in -jN is not passed down to MAKEFLAGS. I installed make 4.2.1 from source, and problem solved.

hanw · 2017-03-14T00:56:16Z

@ChrisDodd, to run a particular test, we can use the name of the p4 program as keyword.
make check-local TESTSUITEFLAGS="-k chain.p4"

In addition, -v will print all commands and stdout/stderr from running the test case:
make check-local TESTSUITEFLAGS="-k chain.p4 -v"

hanw · 2017-03-14T01:03:10Z

We can also narrow down the test case by specifying the name of the test group, e.g.:

make check-local TESTSUITEFLAGS="-k tofino,switch.p4 -v"

-j2 is the default setting on travis

This reverts commit 89a27d7.

…" (#365) This reverts commit 89a27d7.

hanw requested review from ChrisDodd and mihaibudiu March 6, 2017 17:53

mihaibudiu reviewed Mar 6, 2017

View reviewed changes

hanw force-pushed the unittest branch 5 times, most recently from c46815c to 12f3d4b Compare March 7, 2017 21:58

hanw changed the title ~~WIP autotest: add autotest framework for utility unit tests~~ autotest: add autotest framework for utility unit tests Mar 7, 2017

hanw force-pushed the unittest branch 3 times, most recently from ceb4b87 to e31cb45 Compare March 7, 2017 23:57

mihaibudiu approved these changes Mar 8, 2017

View reviewed changes

ChrisDodd reviewed Mar 8, 2017

View reviewed changes

hanw force-pushed the unittest branch 11 times, most recently from fcc3135 to d360b25 Compare March 11, 2017 05:02

Han Wang added 2 commits March 10, 2017 21:24

added autotest testsuite

9a99927

Reorganize existing unit tests into sub-categories.

072a4bb

Removed /unittests, divided existing unit tests into: ir, frontend and util groups.

hanw force-pushed the unittest branch 2 times, most recently from ff7b3e0 to 801be33 Compare March 11, 2017 07:36

add docs/Testing.md

3688f64

cc10512 reviewed Mar 14, 2017

View reviewed changes

improve make check usability

dfdc625

hanw force-pushed the unittest branch from 212eb05 to dfdc625 Compare March 14, 2017 07:05

Han Wang added 13 commits March 14, 2017 01:36

patching testsuite to simplify testsuite.log

fd075a7

keep .test files to make debug simpler

a32c212

avoid duplicate test cases, update xfails.text

49874be

dead code clean up

bc764d1

make check with -j1 on travis

5044612

make check with -j2 instead of -j1

bf42bde

-j2 is the default setting on travis

use p4 program name as keyword for make check-local

f99e53a

fix to disallow -j, only allows -jN

a6ede1b

minor fix

dc77f59

minor clean up

b594362

update Testing section in docs/README.md

4114f9c

make check-local should depend on BUILT_SOURCES and all-am

b706ba3

update .gitignore

f9b6892

hanw force-pushed the unittest branch from 2bdd300 to f9b6892 Compare March 15, 2017 20:35

Merge branch 'master' into unittest

749ec22

hanw merged commit 89a27d7 into p4lang:master Mar 16, 2017

hanw added a commit that referenced this pull request Mar 16, 2017

Revert "autotest: add autotest framework for utility unit tests (#348)"

3d93c1a

This reverts commit 89a27d7.

hanw mentioned this pull request Mar 16, 2017

Revert "autotest: add autotest framework for utility unit tests" #365

Merged

hanw added a commit that referenced this pull request Mar 16, 2017

Revert "autotest: add autotest framework for utility unit tests (#348)…

afec6a7

…" (#365) This reverts commit 89a27d7.

fefer mentioned this pull request Aug 7, 2018

"make" get's 100% without error, but "make check" fails #1433

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

autotest: add autotest framework for utility unit tests #348

autotest: add autotest framework for utility unit tests #348

hanw commented Mar 6, 2017 •

edited

Loading

mihaibudiu Mar 6, 2017

mihaibudiu Mar 6, 2017

hanw Mar 6, 2017

mihaibudiu Mar 6, 2017

mihaibudiu Mar 6, 2017

mihaibudiu Mar 6, 2017

hanw Mar 7, 2017

mihaibudiu Mar 6, 2017

hanw Mar 7, 2017

mihaibudiu Mar 6, 2017

hanw Mar 6, 2017

mihaibudiu Mar 6, 2017

mihaibudiu Mar 6, 2017

hanw commented Mar 7, 2017

mihaibudiu Mar 8, 2017

mihaibudiu Mar 8, 2017

ChrisDodd Mar 8, 2017

ChrisDodd Mar 8, 2017

hanw Mar 8, 2017

ChrisDodd Mar 8, 2017

hanw Mar 8, 2017

ChrisDodd Mar 8, 2017

hanw Mar 8, 2017

hanw commented Mar 13, 2017

ChrisDodd commented Mar 13, 2017 •

edited

Loading

cc10512 commented Mar 13, 2017

hanw commented Mar 14, 2017

cc10512 Mar 14, 2017

hanw Mar 14, 2017

hanw commented Mar 14, 2017

hanw commented Mar 14, 2017 •

edited

Loading

		@@ -0,0 +1,89 @@
		# Copyright 2013-present Barefoot Networks, Inc.

		@@ -0,0 +1,22 @@
		AT_BANNER([intermediate representation unit tests])

autotest: add autotest framework for utility unit tests #348

autotest: add autotest framework for utility unit tests #348

Conversation

hanw commented Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanw commented Mar 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanw commented Mar 13, 2017

ChrisDodd commented Mar 13, 2017 • edited Loading

cc10512 commented Mar 13, 2017

hanw commented Mar 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanw commented Mar 14, 2017

hanw commented Mar 14, 2017 • edited Loading

hanw commented Mar 6, 2017 •

edited

Loading

ChrisDodd commented Mar 13, 2017 •

edited

Loading

hanw commented Mar 14, 2017 •

edited

Loading