Review Board 1.7.22


HIVE-3034: Implement [INCLUDE|EXCLUDE]_IF_SYS_PROP_DEFINED() test macro

Review Request #5227 - Created May 24, 2012 and updated

Prasad Mujumdar
trunk
HIVE-3034
Reviewers
hive
carl
hive-git
Support include/exclude macros in the test and build property to trigger inclusion or exclusion of the test based on the macros.
The test macro is of the format [EX|IN]CLUDE_IF_TEST_PROP_DEFINED(property,val1,val2,...)
This will cause the test to be include or excluded based on the test.include.property or test.exclude.property passed to the build command line. For example,
test101.q -
 -- EXCLUDE_IF_TEST_PROP_DEFINED(fooProp, 100)
..
ant -Dtest.exclude.property="foo:100" test

This will cause the test run to exclude the test101.q script
Added dummy test that use the new macro
Posted (May 25, 2012, 7:40 p.m.)

   

  
build-common.xml (Diff revision 1)
 
 
* I think the name should be changed to test.qmacro.exclude.list. Please move these properties to build.properties and include some comments explaining what they do, the format, etc.
build-common.xml (Diff revision 1)
 
 
Where is "foo" getting set? Maybe this should look something like:

<sysproperty key="test.exclude.property"
                      value="foo:1,${test.exclude.property}"/>
* I think the name of the test should reflect what is being tested. Please change the name to something like qtest_macro_exclude_if_prop_defined_x.q, where "x" is something like "undefined_prop", "not_equal", etc.

* Is "foo" defined somewhere? Looking at this test I can't tell what aspect of the feature is being tested, e.g. foo is defined and we expect this to be excluded, foo is undefined and this should not be excluded, etc. Please add comments to make this clear.

* Can the macro handle whitespace in the parameter list, e.g. before and/or after the commas? 
This test fails because the q.out file is in the wrong directory. QTestUtil expects to find this stuff in ql/src/test/results/clientpositive/, and will put it there automatically when tests are run with -Doverwrite=true
Does the name of the macro still make sense? Right now it seems to imply that we're testing to see if (System.getProperty(x) != null), but actually we're checking to see if it's equal to the second parameter. If that's accurate then the name should probably change to (IN|EX)CLUDE_IF_TEST_PROP_EQUALS(name, val).

Please add a comment explaining what is being tested.
This test is currently getting excluded. Was that the intent?