[GitHub] drill issue #904: DRILL-5717: change some date time test cases with specific...
Github user arina-ielchiieva commented on the issue: https://github.com/apache/drill/pull/904 +1, LGTM. ---
[GitHub] drill issue #904: DRILL-5717: change some date time test cases with specific...
Github user vvysotskyi commented on the issue: https://github.com/apache/drill/pull/904 @weijietong, thanks for the pull request, +1 ---
[GitHub] drill issue #904: DRILL-5717: change some date time test cases with specific...
Github user weijietong commented on the issue: https://github.com/apache/drill/pull/904 done ---
[GitHub] drill issue #904: DRILL-5717: change some date time test cases with specific...
Github user weijietong commented on the issue: https://github.com/apache/drill/pull/904 applied the review comments ---
[GitHub] drill issue #904: DRILL-5717: change some date time test cases with specific...
Github user weijietong commented on the issue: https://github.com/apache/drill/pull/904 @vvysotskyi any more advice ? ---
[GitHub] drill issue #904: DRILL-5717: change some date time test cases with specific...
Github user weijietong commented on the issue: https://github.com/apache/drill/pull/904 @vvysotskyi thanks for your help! have done, please review. ---
[GitHub] drill issue #904: DRILL-5717: change some date time test cases with specific...
Github user weijietong commented on the issue: https://github.com/apache/drill/pull/904 @vvysotskyi thanks for your hard work, I have rewritten the corresponding method. But I have not rewritten the other Locate related cases. Since mockup is a white box testing , I payed some tedious work to explore the possible invocation path, maybe the same as you. I think current Local solution is also acceptable. ---
[GitHub] drill issue #904: DRILL-5717: change some date time test cases with specific...
Github user vvysotskyi commented on the issue:
https://github.com/apache/drill/pull/904
@weijietong thanks for the explanation of your problem. I was able to
reproduce it, but also I found working solution. This mock works correctly. The
problem appears when unit test is run with other tests:
[DateTimeZone](https://github.com/JodaOrg/joda-time/blob/ba95735daf79d00ce0928f30701d691f5e029d81/src/main/java/org/joda/time/DateTimeZone.java)
class contains static field
[cDefault](https://github.com/JodaOrg/joda-time/blob/ba95735daf79d00ce0928f30701d691f5e029d81/src/main/java/org/joda/time/DateTimeZone.java#L128)
which is used to receive timezone in `testToDateForTimeStamp()` and in other
tests. When timezone does not set to this field, but method
[getDefault()](https://github.com/JodaOrg/joda-time/blob/ba95735daf79d00ce0928f30701d691f5e029d81/src/main/java/org/joda/time/DateTimeZone.java#L149)
is called, value is taken from `System.getProperty()`.
When the first test that calls this method does not have a mock, it sets a
real value of timezone from `System.getProperty()`. Therefore our test uses a
unmocked value from `cDefault`.
So lets mock `DateTimeZone.getDefault()` method:
```
new MockUp() {
@Mock
public DateTimeZone getDefault() {
return DateTimeZone.UTC;
}
};
```
---
[GitHub] drill issue #904: DRILL-5717: change some date time test cases with specific...
Github user weijietong commented on the issue: https://github.com/apache/drill/pull/904 @vvysotskyi the problem with mocking strategy is that when I run the test case just with the single method `testToDateForTimeStamp` it will pass. but when I run the whole test cases of the `TestCastFunctions ` , the included `testToDateForTimeStamp` method will fail. ---
[GitHub] drill issue #904: DRILL-5717: change some date time test cases with specific...
Github user vvysotskyi commented on the issue: https://github.com/apache/drill/pull/904 @weijietong which problems did you have when tried to mock `System.getProperty()` method? As the example of nice mock of this method, you may use [this test](https://github.com/apache/drill/pull/936/files#diff-b053496903bff0dc154e42604b93b9efR60). Also besides the timezone, property with locale should be mocked. ---
[GitHub] drill issue #904: DRILL-5717: change some date time test cases with specific...
Github user paul-rogers commented on the issue: https://github.com/apache/drill/pull/904 @vvysotskyi, can you take a look at the revisions and see if it looks OK? ---
[GitHub] drill issue #904: DRILL-5717: change some date time test cases with specific...
Github user weijietong commented on the issue: https://github.com/apache/drill/pull/904 @vvysotskyi thanks for your patient review . As you pointed out , the timezone would not take effect when test cases run in parallel. I also tried the mock strategy. It will also fail in parallel. So I moved out that part of code to a new class called `TestCastFunctionsSpecTZCases` by specifying the timezone before the cluster starts up. It would be clear to the codes. ---
[GitHub] drill issue #904: DRILL-5717: change some date time test cases with specific...
Github user weijietong commented on the issue: https://github.com/apache/drill/pull/904 @vvysotskyi has updated the mentioned parts. These [4ea36c3](https://github.com/apache/drill/commit/4ea36c3f18841b229aca5048b7d162ea16bfd5a1), [caa8b78](https://github.com/apache/drill/commit/caa8b78c5c31c44e59b5ff1bdf6f1900d14b1a1a) two links has been deeply thought about . I think they can't be solved using the same approach at current PR. They are too old to lost some function definitions like `casttimestamptz` . `VectorUtil.appendVectorAccessibleContent` 's date format does not retain time zone part. So I suggest they should be kept there or removed. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill issue #904: DRILL-5717: change some date time test cases with specific...
Github user weijietong commented on the issue: https://github.com/apache/drill/pull/904 @vvysotskyi please review the update ones --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
