-
Notifications
You must be signed in to change notification settings - Fork 153
Conversation
resolved conflict pom.xml
c22ae80
to
242dfaf
Compare
048a4cf
to
aa5388b
Compare
aa5388b
to
48a4fc9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amaya382 So sorry for late reviewing. The system test module example almost passed except for test3 case. And I added some comments. Could you check these comments again? After passing test3, it's ready to be merged, I think.
In addition could you update the license header as described here. Please refer HIVEMALL-12.
Thanks!
public void test4() throws Exception { | ||
// test on HiveRunner once only | ||
predictor.expect(Throwable.class); // you can use systemtest w/ other rules | ||
team.set(HQ.fromStatement("invalid queryyy")); // this query throws an exception |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shows Cannot resolve method
. We need to pass dummy expected argument here.
public void test3() throws Exception { | ||
// test on HiveRunner once only | ||
// auto matching by files which name is `test3` in `case/` and `answer/` | ||
team.set(HQ.autoMatchingByFileName("test3", ci)); // unordered test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be
team.set(HQ.autoMatchingByFileName("test3"), ci);
|
||
The above requires following files | ||
|
||
* `systemtest/src/test/resources/hivemall/HogeTest/init/color.tsv` (`systemtest/src/test/resources/${path/to/package}/${className}/init/${fileName}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be better to rename HogeTest
to QuickExample
here.
pink 255 192 203 | ||
``` | ||
|
||
* `systemtest/src/test/resources/hivemall/HogeTest/case/test3` (`systemtest/src/test/resources/${path/to/package}/${className}/case/${fileName}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto: HogeTest
```sql | ||
-- write your hive queries | ||
-- comments like this and multiple queries in one row are allowed | ||
SELECT blue FROM color WHERE name = 'lavender';SELECT green FROM color WHERE name LIKE 'orange%' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to add semicolon (;
) at the end of line, otherwise interpreted as 2 queries.
} | ||
} | ||
|
||
//// execute RawHQ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: Could you align comment format? Especially the number of /
s.
|
||
if (ordered) { | ||
// take order into consideration (like list) | ||
Assert.assertThat(Arrays.asList(answer.split(IO.RD)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assert.assertThat
requires actual as first argument. So please put result
in first argument.
} else { | ||
// not take order into consideration (like multiset) | ||
Assert.assertThat(Arrays.asList(answer.split(IO.RD)), | ||
Matchers.containsInAnyOrder(result.toArray())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I ran QuickExample#test3
described README, I faced below error.
java.lang.AssertionError:
Expected: iterable over ["165", "69"] in any order
but: Not matched: "165 69"
at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
at org.junit.Assert.assertThat(Assert.java:956)
at org.junit.Assert.assertThat(Assert.java:923)
at hivemall.systemtest.runner.SystemTestRunner.matching(SystemTestRunner.java:202)
at hivemall.systemtest.runner.SystemTestTeam.run(SystemTestTeam.java:180)
at hivemall.QuickExample.test3(QuickExample.java:100)
Could you check this matching is working as expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Lewuathe I cannot reproduce it. plz check that answer/test3
uses tab characters as delimiter, since several space characters have got mixed in README(will be fixed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I seemed to have mistake to copy answer/test3
. I confirmed it works as expected. Thanks.
fileUploadCommitRetryLimit = Integer.valueOf(props.getProperty("fileUploadCommitRetryLimit")); | ||
} | ||
|
||
final Properties TDPorps = System.getProperties(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/TDPorps/TDProps/ ?
} | ||
System.setProperties(TDPorps); | ||
|
||
client = System.getProperties().size() == TDPorps.size() ? TDClient.newClient() // use $HOME/.td/td.conf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is System.getProperties().size() == TDPorps.size()
check meaningful?
TDPorps
is already set as system property in L.94. So the size is always same.
@Lewuathe okay, I'll check them in about a week |
Changes Unknown when pulling 62e3588 on amaya382:feature/systemtest into * on myui:master*. |
1 similar comment
Changes Unknown when pulling 62e3588 on amaya382:feature/systemtest into * on myui:master*. |
@amaya382 https://github.com/myui/hivemall/blob/master/bin/format_header.sh is a tool to run header formatting. FYI |
1 similar comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amaya382 Thank you so much for updating!
Though I added a trivial comment, it LGTM! 👍
@@ -175,7 +180,7 @@ public final boolean isImmutableTable(final String tableName) { | |||
} | |||
} | |||
|
|||
////// execute UploadFileHQ | |||
// >>>execute UploadFileHQ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trivial: >>
instead of >>>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Lewuathe >
intends to mean hierarchical structure of argument
@myui Could you check it? If it's okay please merge it. |
Sure. Will do after #385 is being merged. |
@amaya382 moved to apache/incubator-hivemall#5 |
related with #323
Usage and example are written in
systemtest/README.md
, please refer to it