You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@geode.apache.org by "gesterzhou (GitHub)" <gi...@apache.org> on 2018/12/12 01:37:44 UTC

[GitHub] [geode] gesterzhou opened pull request #2990: GEODE-6143: remove PowerMock from DataTypeJUnitTest

Thank you for submitting a contribution to Apache Geode.

@mcmellawatt @jhuynh1 

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?

- [ ] Is your initial contribution a single, squashed commit?

- [ ] Does `gradlew build` run cleanly?

- [ ] Have you written or updated unit tests to verify your changes?

- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?

### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.


[ Full content available at: https://github.com/apache/geode/pull/2990 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] gesterzhou commented on pull request #2990: GEODE-6143: remove PowerMock from DataTypeJUnitTest

Posted by "gesterzhou (GitHub)" <gi...@apache.org>.

You are right. 

I have changed it to @BeforeClass and @AfterClass. Otherwise, it will impact other tests. I have verified both cases. 

[ Full content available at: https://github.com/apache/geode/pull/2990 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] mcmellawatt commented on pull request #2990: GEODE-6143: remove PowerMock from DataTypeJUnitTest

Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.

It'd be nice to refactor `66000` into a `final` local variable, maybe called `testKey`, since it is used throughout the test.

[ Full content available at: https://github.com/apache/geode/pull/2990 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] mcmellawatt commented on pull request #2990: GEODE-6143: remove PowerMock from DataTypeJUnitTest

Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.

Per my point above, could we refactor this `66000` into some variable or field so that it can be used throughout the tests without copy paste?

[ Full content available at: https://github.com/apache/geode/pull/2990 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] mcmellawatt commented on pull request #2990: GEODE-6143: remove PowerMock from DataTypeJUnitTest

Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.

I'm not familiar enough with our serializer to do an effective review on this one, but maybe it should part of a separate commit and we can tag @upthewaterspout, @galen-pivotal or @bschuchardt on it?  I don't think this should be part of this PowerMock removal commit as it is unrelated. 

[ Full content available at: https://github.com/apache/geode/pull/2990 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] mcmellawatt commented on pull request #2990: GEODE-6143: remove PowerMock from DataTypeJUnitTest

Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.

We are no longer writing an `Integer` here using `DataSerializer.writeInt()`, which I think may have been the point of the test given the test name.  Can we remove PowerMock without changing the types of data we are serializing here?

[ Full content available at: https://github.com/apache/geode/pull/2990 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] gesterzhou commented on pull request #2990: GEODE-6143: remove PowerMock from DataTypeJUnitTest

Posted by "gesterzhou (GitHub)" <gi...@apache.org>.

Dan orally agreed to add the new test. 

[ Full content available at: https://github.com/apache/geode/pull/2990 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] mcmellawatt commented on pull request #2990: GEODE-6143: remove PowerMock from DataTypeJUnitTest

Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.

Rather than having this in a static constructor, can we put it in a `@Before` setup() method?  Also, do we need to unregister the instantiator in test cleanup so that it doesn't effect other tests?

[ Full content available at: https://github.com/apache/geode/pull/2990 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] gesterzhou closed pull request #2990: GEODE-6143: remove PowerMock from DataTypeJUnitTest

Posted by "gesterzhou (GitHub)" <gi...@apache.org>.

[ pull request closed by gesterzhou ]

[ Full content available at: https://github.com/apache/geode/pull/2990 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] upthewaterspout commented on pull request #2990: GEODE-6143: remove PowerMock from DataTypeJUnitTest

Posted by "upthewaterspout (GitHub)" <gi...@apache.org>.

I really like using a real object rather than power mock here. But I think the issue is that this we apparently support multiple fixed id sizes - I see there is a byte and short test above. I think the code probably may work differently if the id can fit in an integer. So we probably need to use an object that fits in an integer.

[ Full content available at: https://github.com/apache/geode/pull/2990 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] gesterzhou commented on pull request #2990: GEODE-6143: remove PowerMock from DataTypeJUnitTest

Posted by "gesterzhou (GitHub)" <gi...@apache.org>.

It's fine. There was a TODO similar to GEMFIRE_ENM. I just implement it. The idea is the same. 

It's not worth to introduce another PR. 

[ Full content available at: https://github.com/apache/geode/pull/2990 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] gesterzhou commented on pull request #2990: GEODE-6143: remove PowerMock from DataTypeJUnitTest

Posted by "gesterzhou (GitHub)" <gi...@apache.org>.

We cannot. Without PowerMock, there's no way to do some "expect", "when", "thenReturn" for static methods. 

We have to feed the component with its data and catch its output. In this test, it intends to test DataSerializableFixedID. I definitely can create one dummy class extends DataSerializableFixedID in this case. However, to use an existing DataSerializableFixedID (I used DiskStoreID) is also fine to prove the logic. 

If you really dislike using DiskStoreID, I can create a dummy class here. 

[ Full content available at: https://github.com/apache/geode/pull/2990 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] gesterzhou commented on pull request #2990: GEODE-6143: remove PowerMock from DataTypeJUnitTest

Posted by "gesterzhou (GitHub)" <gi...@apache.org>.

this is not necessary, because it's the same as CustId's 1. We will only use it within this test class. I only need to create a number which is bigger than short can hold. 

[ Full content available at: https://github.com/apache/geode/pull/2990 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org