CodeGen: refactor unit tests using parameterized tests

CodeGen: refactor unit tests using parameterized tests

CodeGen doesn't do anything fancy like fork() or ptrace(), so it's
safe to test using just plain gtest. That also allows us to convert
the tests to use parameterized tests, so we can easily run each test
independently.

While here, also invert the structure a bit because I expect once I
rewrite and simplify CodeGen, a lot of the ProgramTestFuncs to go
away.

Lastly, for simplicity I'm leaving a lot of statements as ASSERT_TRUE
even if they could use ASSERT_EQ, etc.  Again, I'm expecting some of
those to go away, so I'll worry about cleaning them up once the
unneeded asserts are gone.

BUG=414363
Committed: https://crrev.com/531a117bc26a709a89dd1316ebacaf8bb5467e07
Cr-Commit-Position: refs/heads/master@{#302482}

[mdempsky, 2014-10-31 22:58:04]

mdempsky@chromium.org changed reviewers:
+ jln@chromium.org, rickyz@chromium.org

[mdempsky, 2014-10-31 22:58:04]

rickyz: PTAL

[rickyz (no longer on Chrome), 2014-11-01 00:22:05]

Two small comments, but lgtm, nice catch on the ability to just use gtest here

https://codereview.chromium.org/687813003/diff/1/sandbox/linux/seccomp-bpf/co...
File sandbox/linux/seccomp-bpf/codegen_unittest.cc (right):

https://codereview.chromium.org/687813003/diff/1/sandbox/linux/seccomp-bpf/co...
sandbox/linux/seccomp-bpf/codegen_unittest.cc:24: using
CodeGen::CutGraphIntoBasicBlocks;
This is way prettier, but I think using-declaration appears under the banned
C++11 features at http://chromium-cpp.appspot.com/. It looks like it's just
because of MSVS2013 support, which doesn't apply here -  - are we exempt since
this is Linux only? (I hope we are)

https://codereview.chromium.org/687813003/diff/1/sandbox/linux/seccomp-bpf/co...
sandbox/linux/seccomp-bpf/codegen_unittest.cc:41: void TearDown() override {
GetParam()(&gen_, head_, flags_); }
It might just be me, but it's a little weird that the meat of the test happens
inside of TearDown. Instead of ending each test with head_ = ..., maybe we could
just get rid of head_ and flags_ member variables and just call
GetParam()(&gen_, head, NO_FLAGS|HAS_MERGEABLE_TAILS) at the end of each of the
tests?

[mdempsky, 2014-11-01 00:59:54]

rickyz: Thanks, updated. PTAL!

https://codereview.chromium.org/687813003/diff/1/sandbox/linux/seccomp-bpf/co...
File sandbox/linux/seccomp-bpf/codegen_unittest.cc (right):

https://codereview.chromium.org/687813003/diff/1/sandbox/linux/seccomp-bpf/co...
sandbox/linux/seccomp-bpf/codegen_unittest.cc:24: using
CodeGen::CutGraphIntoBasicBlocks;
On 2014/11/01 00:22:05, rickyz wrote:
> This is way prettier, but I think using-declaration appears under the banned
> C++11 features at http://chromium-cpp.appspot.com/. It looks like it's just
> because of MSVS2013 support, which doesn't apply here -  - are we exempt since
> this is Linux only? (I hope we are)

That prohibition is only against 'using' declarations that specify the base
class's constructor (which is new to C++11).  This form of 'using' declarations
(for base member functions) was already supported by C++98.

https://codereview.chromium.org/687813003/diff/1/sandbox/linux/seccomp-bpf/co...
sandbox/linux/seccomp-bpf/codegen_unittest.cc:41: void TearDown() override {
GetParam()(&gen_, head_, flags_); }
On 2014/11/01 00:22:05, rickyz wrote:
> It might just be me, but it's a little weird that the meat of the test happens
> inside of TearDown. Instead of ending each test with head_ = ..., maybe we
could
> just get rid of head_ and flags_ member variables and just call
> GetParam()(&gen_, head, NO_FLAGS|HAS_MERGEABLE_TAILS) at the end of each of
the
> tests?

Fair enough.  I added a RunTest() function to share the common logic, and now
call it explicitly within each test.

I also created MakeInstruction() helper functions to sanity check the
instruction fields.  (Not much of a win now, but it's going to help later when I
get rid of Instruction.)

[mdempsky, 2014-11-03 18:03:55]

jln: Should be ready for you to review too.

[jln (very slow on Chromium), 2014-11-03 20:19:56]

This lgtm if you prefer to go this route (which seems reasonable), but we wary
of GTest.

- Simple things such "having started threads" (i.e. starting and joining a
thread) can leak into the next test if that tests needs to look at
/proc/self/task/ for instance. See unit_tests.cc:132 for the workaround.

I agree with your statement that CodeGen should be fine though.

- GTest's assertions ASSERT_* don't work as expected from subfunctions (they
simply return one level). SANDBOX_ASSERT() works as expected.

- We may want to support parametrized SANDBOX_TEST in the future. But having yet
more macro magic deterred me in the past. We could also add support for
process-isolated tests in GTest.

PS: for BPF_ASSERT, which is the same as  SANDBOX_ASSERT, we added BPF_ASSERT_EQ
etc.. We should  do the same for SANDBOX_ASSERT.

https://codereview.chromium.org/687813003/diff/40001/sandbox/linux/seccomp-bp...
File sandbox/linux/seccomp-bpf/codegen_unittest.cc (right):

https://codereview.chromium.org/687813003/diff/40001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/codegen_unittest.cc:52: EXPECT_TRUE(ret);
EXPECT_NE?

Also this should be an ASSERT....

...but, one issue though is that with GTest, ASSERTs only leave the current
function, but the calling function will continue to run normally (that's because
they can't use exceptions).

With SANDBOX_TEST, we worked around this since our assert could simply _exit().

https://codereview.chromium.org/687813003/diff/40001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/codegen_unittest.cc:72: EXPECT_TRUE(ret);
EXPECT_NE?

[mdempsky, 2014-11-03 20:43:00]

> but we wary of GTest.

Acknowledged.

https://codereview.chromium.org/687813003/diff/40001/sandbox/linux/seccomp-bp...
File sandbox/linux/seccomp-bpf/codegen_unittest.cc (right):

https://codereview.chromium.org/687813003/diff/40001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/codegen_unittest.cc:52: EXPECT_TRUE(ret);
On 2014/11/03 20:19:56, jln wrote:
> EXPECT_NE?

Done.

> Also this should be an ASSERT....
> 
> ...but, one issue though is that with GTest, ASSERTs only leave the current
> function, but the calling function will continue to run normally (that's
because
> they can't use exceptions).

Yeah, the other issue is ASSERT can't be used in a function that returns
non-void, hence why I'm using EXPECT here. :(

https://codereview.chromium.org/687813003/diff/40001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/codegen_unittest.cc:72: EXPECT_TRUE(ret);
On 2014/11/03 20:19:56, jln wrote:
> EXPECT_NE?

Done.

[mdempsky, 2014-11-03 20:43:08]

The CQ bit was checked by mdempsky@chromium.org

[commit-bot: I haz the power, 2014-11-03 20:44:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/687813003/60001

[commit-bot: I haz the power, 2014-11-03 21:26:55]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-11-03 21:27:34]

Patchset 4 (id:??) landed as
https://crrev.com/531a117bc26a709a89dd1316ebacaf8bb5467e07
Cr-Commit-Position: refs/heads/master@{#302482}