ARM64: Refactor instruction selection unit tests.

ARM64: Refactor instruction selection unit tests.

BUG=
R=bmeurer@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=23188

[Benedikt Meurer, 2014-08-14 17:58:31]

https://codereview.chromium.org/475823002/diff/1/test/compiler-unittests/arm6...
File test/compiler-unittests/arm64/instruction-selector-arm64-unittest.cc
(right):

https://codereview.chromium.org/475823002/diff/1/test/compiler-unittests/arm6...
test/compiler-unittests/arm64/instruction-selector-arm64-unittest.cc:82: typedef
InstructionSelectorTestWithParam<DPI> LogicalTest;
s/LogicalTest/InstructionSelectorLogicalTest/

See below for an explanation.

https://codereview.chromium.org/475823002/diff/1/test/compiler-unittests/arm6...
test/compiler-unittests/arm64/instruction-selector-arm64-unittest.cc:85:
TEST_P(LogicalTest, Parameter) {
Test names must be globally unique, so please always add the InstructionSelector
prefix to the test case name, i.e. InstructionSelectorLogicalTest.

This has the additional advantage that one can run all InstructionSelector tests
by using --gtest_filter=InstructionSelector* with the compiler-unittests binary.

[Rodolph Perfetta, 2014-08-14 18:29:45]

https://codereview.chromium.org/475823002/diff/1/test/compiler-unittests/arm6...
File test/compiler-unittests/arm64/instruction-selector-arm64-unittest.cc
(right):

https://codereview.chromium.org/475823002/diff/1/test/compiler-unittests/arm6...
test/compiler-unittests/arm64/instruction-selector-arm64-unittest.cc:85:
TEST_P(LogicalTest, Parameter) {
My understanding was that the name will be concatenated with the first parameter
of INSTANTIATE_TEST_CASE_P, so here InstructionSelectorTest. If I run
compiler-unittest, those tests are currently listed as 
InstructionSelectorTest/LogicalTest.Parameter and using
--gtest_filter='InstructionSelector*' will find them.

Have I misunderstood?

[Benedikt Meurer, 2014-08-14 18:31:07]

>
https://codereview.chromium.org/475823002/diff/1/test/compiler-unittests/arm6...
> test/compiler-unittests/arm64/instruction-selector-arm64-unittest.cc:85:
TEST_P(LogicalTest, Parameter) {
> My understanding was that the name will be concatenated with the first
parameter of INSTANTIATE_TEST_CASE_P, so here InstructionSelectorTest. If I run
compiler-unittest, those tests are currently listed as 
InstructionSelectorTest/LogicalTest.Parameter and using
--gtest_filter='InstructionSelector*' will find them.
> 
> Have I misunderstood?

Ah right, of course. Sorry, I confused myself :-)

LGTM

[Benedikt Meurer, 2014-08-14 18:33:17]

On 2014/08/14 at 18:31:07, Benedikt Meurer wrote:
> >
https://codereview.chromium.org/475823002/diff/1/test/compiler-unittests/arm6...
> > test/compiler-unittests/arm64/instruction-selector-arm64-unittest.cc:85:
TEST_P(LogicalTest, Parameter) {
> > My understanding was that the name will be concatenated with the first
parameter of INSTANTIATE_TEST_CASE_P, so here InstructionSelectorTest. If I run
compiler-unittest, those tests are currently listed as 
InstructionSelectorTest/LogicalTest.Parameter and using
--gtest_filter='InstructionSelector*' will find them.
> > 
> > Have I misunderstood?
> 
> Ah right, of course. Sorry, I confused myself :-)

Ahem, still my first statement was correct. The class name will be composed of
LogicalTest and Parameter, which may not be unique among all tests in
compiler-unittests. So better add the InstructionSelector prefix here as well.

[Rodolph Perfetta (ARM), 2014-08-15 10:06:57]

https://codereview.chromium.org/475823002/diff/1/test/compiler-unittests/arm6...
File test/compiler-unittests/arm64/instruction-selector-arm64-unittest.cc
(right):

https://codereview.chromium.org/475823002/diff/1/test/compiler-unittests/arm6...
test/compiler-unittests/arm64/instruction-selector-arm64-unittest.cc:82: typedef
InstructionSelectorTestWithParam<DPI> LogicalTest;
On 2014/08/14 17:58:31, Benedikt Meurer wrote:
> s/LogicalTest/InstructionSelectorLogicalTest/
> 
> See below for an explanation.

Done.

https://codereview.chromium.org/475823002/diff/1/test/compiler-unittests/arm6...
test/compiler-unittests/arm64/instruction-selector-arm64-unittest.cc:85:
TEST_P(LogicalTest, Parameter) {
On 2014/08/14 17:58:31, Benedikt Meurer wrote:
> Test names must be globally unique, so please always add the
InstructionSelector
> prefix to the test case name, i.e. InstructionSelectorLogicalTest.
> 
> This has the additional advantage that one can run all InstructionSelector
tests
> by using --gtest_filter=InstructionSelector* with the compiler-unittests
binary.

Acknowledged.

[Benedikt Meurer, 2014-08-18 04:23:20]

LGTM

You can land TF changes even while the tree is throttled.

[Benedikt Meurer, 2014-08-18 06:38:20]

Ok, I've looked into this again, and I think you are right. We can avoid the
possible name collision using an anonymous namespace, i.e.


namespace {

class MyTest : public InstructionSelectorTestWithParam<ParamType> {};

TEST_P(MyTest, A) {...}
TEST_P(MyTest, B) {...}

}

INSTANTIATE_TEST_CASE_P(InstructionSelectorTest, MyTest,
::testing::Values(...));


I really like this solution (shortens the test names), and I'll apply that to
the ARM instruction selector unittest.

[Rodolph Perfetta (ARM), 2014-08-18 12:50:05]

On 2014/08/18 06:38:20, Benedikt Meurer wrote:
> Ok, I've looked into this again, and I think you are right. We can avoid the
> possible name collision using an anonymous namespace, i.e.
> 
> 
> namespace {
> 
> class MyTest : public InstructionSelectorTestWithParam<ParamType> {};
> 
> TEST_P(MyTest, A) {...}
> TEST_P(MyTest, B) {...}
> 
> }
> 
> INSTANTIATE_TEST_CASE_P(InstructionSelectorTest, MyTest,
> ::testing::Values(...));
> 
> 
> I really like this solution (shortens the test names), and I'll apply that to
> the ARM instruction selector unittest.

Done for arm64.

[Rodolph Perfetta (ARM), 2014-08-19 10:03:04]

re-refactored ;-)

[Benedikt Meurer, 2014-08-19 10:12:55]

lgtm

[Rodolph Perfetta (ARM), 2014-08-19 10:14:35]

Committed patchset #4 manually as 23188 (presubmit successful).