add an example gtest case, useful for unit-testing compiler pass #434

hanw · 2017-04-04T23:16:13Z

No description provided.

mihaibudiu

This example does not show why doing tests this way is useful.

mihaibudiu · 2017-04-04T23:18:07Z

test/gtest/helper.h

+#include <string>
+
+/* preprocessing by prepending the content of core.p4 to test program */
+std::string with_core_p4 (std::string pgm) {


Maybe this should be read from the file?

Again, since you don't mutate pgm, it's probably better to make it a const std::string&.

I agree it'd be best to read core.p4 from the file once when the GTest executable starts (so in main()). We can then reuse it for every invocation of with_core_p4. If that's not quick to do, though, I think it'd be fine to do this now and file a followup issue to fix that later.

mihaibudiu · 2017-04-04T23:19:13Z

test/gtest/typecheck_test.cpp

@@ -0,0 +1,62 @@
+/*


This is more or less what the compiler is doing now.
It would be much easier to add a flag for the compiler to stop compiling after a certain pass.

For unit tests, we want as much control as possible. I think we're better off using an approach like this. It gives us maximum flexibility, and it has the advantage of not invoking cpp for every unit test.

sethfowler · 2017-04-04T23:23:31Z

frontends/p4/p4-parse.ypp

@@ -993,6 +993,18 @@ const IR::P4Program *parse_P4_16_file(const char *name, FILE *in) {
    return new IR::P4Program(declarations->srcInfo, declarations);
 }

+const IR::P4Program *parse_string(std::string &pgm) {


const std::string& is probably preferable, since it doesn't look like you mutate pgm in here.

sethfowler

Looks good! Just needs a few tweaks.

sethfowler · 2017-04-04T23:28:40Z

test/gtest/helper.h

+#include <string>
+
+/* preprocessing by prepending the content of core.p4 to test program */
+std::string with_core_p4 (std::string pgm) {


Again, since you don't mutate pgm, it's probably better to make it a const std::string&.

I agree it'd be best to read core.p4 from the file once when the GTest executable starts (so in main()). We can then reuse it for every invocation of with_core_p4. If that's not quick to do, though, I think it'd be fine to do this now and file a followup issue to fix that later.

sethfowler · 2017-04-04T23:30:35Z

test/gtest/typecheck_test.cpp

@@ -0,0 +1,62 @@
+/*


For unit tests, we want as much control as possible. I think we're better off using an approach like this. It gives us maximum flexibility, and it has the advantage of not invoking cpp for every unit test.

sethfowler · 2017-04-04T23:33:09Z

test/gtest/typecheck_test.cpp

+        "parser Parser<H, M> (packet_in p){ state start{} };\n"
+        "control empty() { apply {} };\n"
+        "package top(empty e);\n"
+        "top(empty()) main;\n");


C++11's raw string literals might make this a little nicer. (And we can use them!)

sethfowler · 2017-04-04T23:34:32Z

test/gtest/typecheck_test.cpp

+        "top(empty()) main;\n");
+
+    const IR::P4Program* pgm = parse_string(program);
+    ASSERT_TRUE(pgm != nullptr);


ASSERT_NE(nullptr, pgm); is better; GTest can give much nicer log messages in that case.

sethfowler · 2017-04-04T23:34:53Z

test/gtest/typecheck_test.cpp

+
+    pgm = pgm->apply(passes);
+
+    ASSERT_TRUE(pgm != nullptr);


Same as above.

sethfowler · 2017-04-04T23:39:36Z

@mbudiu-vmw, this kind of test is useful for a number of reasons:

They give us fine-grained control over the testing environment, which passes are run, and the like. This allows us to test things that would be difficult or impossible to test if we had to run the test case through the entire compiler.
They let us assert things that we can only assert if we have access to the IR and other runtime data structures.
They let us generate tests programmatically in a very flexible way, without the complexity of using external Python scripts or the like.

In addition, tests written this way have less overhead than our current approach to writing tests, which will speed up our test suite over the long term.

mihaibudiu · 2017-04-04T23:46:34Z

However;

you will write lots of complete P4 programs into text strings, without any support from syntax highlighting, which will be harder to maintain
in general it is very difficult to run passes out of the standard order
in general it is very difficult to write invariant checks over the IR, because it is a large and complicated object
you will pay in compiling and linking time; in the end I am not sure it will be faster. On this last point, every time you touch a makefile everything is being rebuilt, which is very slow. If you want to write tests in C++ you should consider fixing this problem.

One would need some sort of pattern-matching language for writing readable IR-based tests.

sethfowler · 2017-04-04T23:53:07Z

It's true that you won't have syntax highlighting, but some tradeoffs always have to be made.

Regarding the issue of everything being rebuilt when you touch a Makefile, that is true, but we can easily have as many tests as we want in a single source file. Adding a new GTest will often not require touching a Makefile at all. Right now we don't have a lot of GTests, so new tests generally don't have an existing file that they'd logically belong in, but over time I expect that most tests will find a natural home in an existing file.

antoninbas · 2017-04-04T23:57:23Z

Regarding @mbudiu-vmw's first point you will write lots of complete P4 programs into text strings, without any support from syntax highlighting, which will be harder to maintain, I believe that for unit cases that need P4 input, you should try to get that input from a file, not hardcode it in the unit test cpp source.

sethfowler · 2017-04-04T23:59:59Z

I think there are many times when that'd be appropriate, but there are massive readability benefits to seeing the P4 code right there inside the test, so for short snippets I'd actually prefer this approach. We should make the decision on a case-by-case basis, I think.

hanw · 2017-04-05T00:01:50Z

What I don't yet understand is how in-complete p4 programs are tested in testdata/p4_16_samples/.

For example,

./p4test --p4v 16 ../testdata/p4_16_samples/spec-ex01.p4 
warning: Program does not contain a `main' module

hanw · 2017-04-05T00:07:21Z

I agree with @antoninbas that if a p4 program is complete, then it should be read from a file, and tested with existing test scripts. But if a program is a fragment, then unit-test would be helpful.

sethfowler · 2017-04-05T00:13:49Z

test/gtest/helper.h

@@ -17,7 +17,7 @@ limitations under the License.
 #include <string>

 /* preprocessing by prepending the content of core.p4 to test program */
-std::string with_core_p4 (std::string pgm) {
+std::string with_core_p4 (const std::string pgm) {


const std::string& please. =)

mihaibudiu · 2017-04-05T00:20:12Z

You can't really test incomplete programs.
The program you wrote is complete, but does not have a main, so compilation stops after the front-end.
The program is still used to generate some intermediate files which are used for checking that the intermediate passes are correct. The test scripts dump P4 out of the program at 3 points in the compilation chain, and save these to files, which are compared next time the program is compiled.

sethfowler · 2017-04-05T00:22:39Z

test/gtest/helper.h

@@ -17,7 +17,7 @@ limitations under the License.
 #include <string>

 /* preprocessing by prepending the content of core.p4 to test program */
-std::string with_core_p4 (const std::string pgm) {
+std::string with_core_p4 (const std::string& pgm) {


…r each run

ChrisDodd · 2017-04-05T17:41:05Z

test/gtest/helper.h

+
+/* preprocessing by prepending the content of core.p4 to test program */
+std::string with_core_p4 (const std::string& pgm) {
+    std::ifstream input("p4include/core.p4");


This relies on running the test in the build directory -- since that is what generally happens, perhaps ok?

Yes, I don't think make check works outside the build directory.

gtest will work in both p4c and p4c/build.

add an example gtest case, useful for unit-testing compiler passes

16e8f68

hanw requested review from sethfowler and ChrisDodd April 4, 2017 23:16

mihaibudiu reviewed Apr 4, 2017

View reviewed changes

sethfowler reviewed Apr 4, 2017

View reviewed changes

const param

4e8a045

sethfowler approved these changes Apr 4, 2017

View reviewed changes

addressed comment

4e9a60c

sethfowler reviewed Apr 5, 2017

View reviewed changes

minor fix

a37532c

sethfowler reviewed Apr 5, 2017

View reviewed changes

Han Wang and others added 3 commits April 5, 2017 08:02

read core.p4 from file instead, clean up global variable in yacc afte…

4bb7f32

…r each run

make cpplint happy

b062a9b

Merge branch 'master' into master

37d06d0

hanw merged commit 4fdef50 into p4lang:master Apr 5, 2017

ChrisDodd reviewed Apr 5, 2017

View reviewed changes

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

add an example gtest case, useful for unit-testing compiler pass #434

add an example gtest case, useful for unit-testing compiler pass #434

hanw commented Apr 4, 2017

mihaibudiu left a comment

mihaibudiu Apr 4, 2017

sethfowler Apr 4, 2017

antoninbas Apr 4, 2017

mihaibudiu Apr 4, 2017

sethfowler Apr 4, 2017

sethfowler Apr 4, 2017

sethfowler left a comment

sethfowler Apr 4, 2017

sethfowler Apr 4, 2017

sethfowler Apr 4, 2017

sethfowler Apr 4, 2017

sethfowler Apr 4, 2017

sethfowler commented Apr 4, 2017

mihaibudiu commented Apr 4, 2017

sethfowler commented Apr 4, 2017

antoninbas commented Apr 4, 2017

sethfowler commented Apr 4, 2017

hanw commented Apr 5, 2017

hanw commented Apr 5, 2017

sethfowler Apr 5, 2017

mihaibudiu commented Apr 5, 2017

sethfowler Apr 5, 2017

ChrisDodd Apr 5, 2017

hanw Apr 5, 2017

hanw Apr 5, 2017

add an example gtest case, useful for unit-testing compiler pass #434

add an example gtest case, useful for unit-testing compiler pass #434

Conversation

hanw commented Apr 4, 2017

mihaibudiu left a 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

sethfowler left a 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

sethfowler commented Apr 4, 2017

mihaibudiu commented Apr 4, 2017

sethfowler commented Apr 4, 2017

antoninbas commented Apr 4, 2017

sethfowler commented Apr 4, 2017

hanw commented Apr 5, 2017

hanw commented Apr 5, 2017

Choose a reason for hiding this comment

mihaibudiu commented Apr 5, 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