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

Posted to notifications@geode.apache.org by "PurelyApplied (GitHub)" <gi...@apache.org> on 2018/10/11 19:06:31 UTC

[GitHub] [geode] PurelyApplied opened pull request #2593: GEODE-5850: Do not build jar in test.

Thank you for submitting a contribution to Apache Geode.

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

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

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

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

- [x] Does `gradlew build` run cleanly?

- [n/a - existing test failures] Have you written or updated unit tests to verify your changes?

- [n/a] 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/2593 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] PurelyApplied commented on pull request #2593: GEODE-5850: Do not build jar in test.

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

Similar to the other comment, there's no real reason this has to get packaged in the `jar` to the full `o.a.g.m.i.c.c` package.  Would this test read cleaner if the jar put the `SerializableNameContainer` at the top-level package?

I know I'm overthinking this.

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

[GitHub] [geode] PurelyApplied commented on issue #2593: GEODE-5850: Do not build jar in test.

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

@pivotal-jbarrett #2593 and #2603 are addressing similar failures in different tests, yes.  I should've clarified the commit message / header.

@jdeppe-pivotal I fail to see what about `PutCommandWithJsonTest.java` suggests it has to do with a build step.  It seems like a test of the `put` command to me, which requires a class of a toy datatype to use in that `put`.  I think it's more that this jar is a necessary resource for this test to execute, and that such a resource should be built during the build steps.  But at the same time, I do agree that it's somewhat  cumbersome.  I just didn't see another direct solution to this Java version conflict.  (Except that...)

@jinmeiliao I didn't notice that, but that makes sense.  I guess the real problem is that our pipeline needs to set `GF_HOME`, too.

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

[GitHub] [geode] PurelyApplied commented on pull request #2593: GEODE-5850: Do not build jar in test.

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

Any opinions on where this data container class should live?  I kept it on the path to be the same as the test that needs it, but that's not really necessary.  It could belong to `geode-dunit` as a shared resource, if we track down other places that need a simple `java.io.Serializable` data container class.

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

[GitHub] [geode] jinmeiliao commented on issue #2593: GEODE-5850: Do not build jar in test.

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

I do prefer to build the jar in the test since that jar very test specific. We don't really need to have class file lying around in our source code.

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

[GitHub] [geode] jinmeiliao commented on issue #2593: GEODE-5850: Do not build jar in test.

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

I couldn't get this failure reproduced in my environment. In the pipeline, it looks the server/locators are running using java1.8. I think we need to run it using jdk11.

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

[GitHub] [geode] jinmeiliao commented on issue #2593: GEODE-5850: Do not build jar in test.

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

Oh, missed reading the ticket. Maybe the JarBuilder would have some hooks to specify the target compatibility when compiling the code.

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

[GitHub] [geode] PurelyApplied commented on pull request #2593: GEODE-5850: Do not build jar in test.

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

Upon immediate hindsight, this is absolutely not the right way to do this.  Maybe examine the class, get a list of dependencies, and depend on those.  As it stands, our `repeat-new-tests.sh` will run the entire `*Test` instead of just repeating the changed tests within that scope.

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

[GitHub] [geode] PurelyApplied commented on issue #2593: GEODE-5850: Do not build jar in test.

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

@jinmeiliao I think you missed the scope of the ticket?  We explicitly cannot do that in Java11, since the test VM would compile it in Java11, making it incompatible with our Java8-compiled server code.

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

[GitHub] [geode] PurelyApplied closed pull request #2593: GEODE-5850: Do not build jar in test.

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

[ pull request closed by PurelyApplied ]

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

[GitHub] [geode] pivotal-jbarrett commented on issue #2593: GEODE-5850: Do not build jar in test.

Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.

I am confused. Are both #2603 and #2593 necessary?


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

[GitHub] [geode] jinmeiliao commented on issue #2593: GEODE-5850: Do not build jar in test.

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

I do believe that when the build is fixed to use jdk11 to start server/locators, these changes are no longer necessary. 

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