----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5227/#review8117 -----------------------------------------------------------
build-common.xml <https://reviews.apache.org/r/5227/#comment17635> * 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 <https://reviews.apache.org/r/5227/#comment17640> Where is "foo" getting set? Maybe this should look something like: <sysproperty key="test.exclude.property" value="foo:1,${test.exclude.property}"/> ql/src/test/queries/clientpositive/join91.q <https://reviews.apache.org/r/5227/#comment17636> * 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? ql/src/test/queries/clientpositive/join91.q.out <https://reviews.apache.org/r/5227/#comment17637> 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 ql/src/test/queries/clientpositive/join92.q <https://reviews.apache.org/r/5227/#comment17638> 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. ql/src/test/queries/clientpositive/join99.q <https://reviews.apache.org/r/5227/#comment17639> This test is currently getting excluded. Was that the intent? - Carl On 2012-05-24 21:30:17, Prasad Mujumdar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5227/ > ----------------------------------------------------------- > > (Updated 2012-05-24 21:30:17) > > > Review request for hive and Carl Steinbach. > > > Summary > ------- > > 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 > > > This addresses bug HIVE-3034. > https://issues.apache.org/jira/browse/HIVE-3034 > > > Diffs > ----- > > build-common.xml b2135b3 > ql/src/test/org/apache/hadoop/hive/ql/QTestUtil.java 93d002a > ql/src/test/queries/clientpositive/join91.q PRE-CREATION > ql/src/test/queries/clientpositive/join91.q.out PRE-CREATION > ql/src/test/queries/clientpositive/join92.q PRE-CREATION > ql/src/test/queries/clientpositive/join99.q PRE-CREATION > ql/src/test/queries/clientpositive/join99.q.out PRE-CREATION > > Diff: https://reviews.apache.org/r/5227/diff > > > Testing > ------- > > Added dummy test that use the new macro > > > Thanks, > > Prasad > >
