Non-SFI NaCl: Stop running sandbox tests in components_unittests

Non-SFI NaCl: Stop running sandbox tests in components_unittests

Also revert the temporary change for the sandbox test framework,
which allows fork() with threads.

https://codereview.chromium.org/242383003

TEST=nacl_loader_unittests, components_unittests, and trybots
BUG=364751
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269558

[hamaji, 2014-05-07 06:52:51]

nacl_loader_unittests is running in both the waterfall and trybots. I think we
want two more (small) infra changes:

1. Run nacl_loader_unittests on more builders:
https://codereview.chromium.org/270623004/
2. Make nacl_loader_unittests a criteria of LKGR.

But I think it's OK to get this reviewed now.

https://codereview.chromium.org/273623002/diff/1/sandbox/linux/tests/unit_tes...
File sandbox/linux/tests/unit_tests.cc (right):

https://codereview.chromium.org/273623002/diff/1/sandbox/linux/tests/unit_tes...
sandbox/linux/tests/unit_tests.cc:124: // The kernel is at liberty to wake a
thread id futex before updating /proc.
I guess we want to keep this?

[hamaji, 2014-05-08 00:27:32]

Update:

1: landed (https://codereview.chromium.org/270623004/)
2: sent a change for review (https://codereview.chromium.org/267293008)

[jln (very slow on Chromium), 2014-05-08 00:36:27]

Great, thanks for wrapping this up!
lgtm

[jln (very slow on Chromium), 2014-05-08 00:36:36]

https://chromiumcodereview.appspot.com/273623002/diff/1/sandbox/linux/tests/u...
File sandbox/linux/tests/unit_tests.cc (right):

https://chromiumcodereview.appspot.com/273623002/diff/1/sandbox/linux/tests/u...
sandbox/linux/tests/unit_tests.cc:124: // The kernel is at liberty to wake a
thread id futex before updating /proc.
On 2014/05/07 06:52:51, hamaji wrote:
> I guess we want to keep this?

Ohh yeah absolutely. If a test starts and stops a thread, we need to wait until
it disappears.

[hamaji, 2014-05-08 03:49:53]

The CQ bit was checked by hamaji@chromium.org

[commit-bot: I haz the power, 2014-05-08 03:52:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hamaji@chromium.org/273623002/1

[commit-bot: I haz the power, 2014-05-08 04:47:05]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  chromium_presubmit on tryserver.chromium

[commit-bot: I haz the power, 2014-05-08 05:02:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-08 05:02:02]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[hamaji, 2014-05-08 05:15:04]

Ah, I needed Mark's review.

[Mark Seaborn, 2014-05-08 15:29:38]

LGTM

https://codereview.chromium.org/273623002/diff/1/sandbox/linux/tests/main.cc
File sandbox/linux/tests/main.cc (right):

https://codereview.chromium.org/273623002/diff/1/sandbox/linux/tests/main.cc#...
sandbox/linux/tests/main.cc:12: namespace sandbox {
Can this "namespace sandbox" be dropped now?  It only wraps an anon namespace.

https://codereview.chromium.org/273623002/diff/1/sandbox/linux/tests/unit_tes...
File sandbox/linux/tests/unit_tests.cc (right):

https://codereview.chromium.org/273623002/diff/1/sandbox/linux/tests/unit_tes...
sandbox/linux/tests/unit_tests.cc:140: << "flaky.\n";
Nit: Isn't '\n' unnecessary for ASSERT_EQ?  Also fits on previous line?

[jln (DO NOT USE THIS), 2014-05-08 16:30:02]

https://codereview.chromium.org/273623002/diff/1/sandbox/linux/tests/main.cc
File sandbox/linux/tests/main.cc (right):

https://codereview.chromium.org/273623002/diff/1/sandbox/linux/tests/main.cc#...
sandbox/linux/tests/main.cc:12: namespace sandbox {
On 2014/05/08 15:29:39, Mark Seaborn wrote:
> Can this "namespace sandbox" be dropped now?  It only wraps an anon namespace.

Yeah, but it's the right idiom in sandbox/, no?. Everything should always be in
namespace sandbox.
It just so happens that main.cc is only declaring one exported function and it's
outside of the namespace for obvious reasons.

But typically sandbox/ should not have to add sandbox::.

https://codereview.chromium.org/273623002/diff/1/sandbox/linux/tests/unit_tes...
File sandbox/linux/tests/unit_tests.cc (right):

https://codereview.chromium.org/273623002/diff/1/sandbox/linux/tests/unit_tes...
sandbox/linux/tests/unit_tests.cc:140: << "flaky.\n";
On 2014/05/08 15:29:39, Mark Seaborn wrote:
> Nit: Isn't '\n' unnecessary for ASSERT_EQ?  Also fits on previous line?

Ohh good catch!

[Mark Seaborn, 2014-05-08 16:42:14]

https://codereview.chromium.org/273623002/diff/1/sandbox/linux/tests/main.cc
File sandbox/linux/tests/main.cc (right):

https://codereview.chromium.org/273623002/diff/1/sandbox/linux/tests/main.cc#...
sandbox/linux/tests/main.cc:12: namespace sandbox {
On 2014/05/08 16:30:02, jln (DO NOT USE THIS) wrote:
> On 2014/05/08 15:29:39, Mark Seaborn wrote:
> > Can this "namespace sandbox" be dropped now?  It only wraps an anon
namespace.
> 
> Yeah, but it's the right idiom in sandbox/, no?. Everything should always be
in
> namespace sandbox.
> It just so happens that main.cc is only declaring one exported function and
it's
> outside of the namespace for obvious reasons.
> 
> But typically sandbox/ should not have to add sandbox::.

Oh, I see, yes, it does make a semantic difference.  Ignore my comment, then.

[hamaji, 2014-05-10 08:06:24]

https://codereview.chromium.org/273623002/diff/1/sandbox/linux/tests/unit_tes...
File sandbox/linux/tests/unit_tests.cc (right):

https://codereview.chromium.org/273623002/diff/1/sandbox/linux/tests/unit_tes...
sandbox/linux/tests/unit_tests.cc:140: << "flaky.\n";
On 2014/05/08 16:30:02, jln (DO NOT USE THIS) wrote:
> On 2014/05/08 15:29:39, Mark Seaborn wrote:
> > Nit: Isn't '\n' unnecessary for ASSERT_EQ?  Also fits on previous line?
> 
> Ohh good catch!

Done. After I reverted https://codereview.chromium.org/242383003, I didn't look
at the code carefully :(

[hamaji, 2014-05-10 08:06:58]

The CQ bit was checked by hamaji@chromium.org

[commit-bot: I haz the power, 2014-05-10 08:07:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hamaji@chromium.org/273623002/20001

[commit-bot: I haz the power, 2014-05-10 11:38:58]

Change committed as 269558