You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "jinmeiliao (GitHub)" <gi...@apache.org> on 2018/12/06 19:11:02 UTC
[GitHub] [geode] jinmeiliao opened pull request #2962: GEODE-5971:
refactor StartJConsoleCommand to use ResultModel
Co-authored-by: Peter Tran <pt...@pivotal.io>
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:
- [ ] 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/2962 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] aditya87 commented on pull request #2962:
GEODE-5971: refactor StartJConsoleCommand to use ResultModel
Posted by "aditya87 (GitHub)" <gi...@apache.org>.
Curious, do we not have more end-to-end test coverage for this command?
[ Full content available at: https://github.com/apache/geode/pull/2962 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #2962:
GEODE-5971: refactor StartJConsoleCommand to use ResultModel
Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.
These commands are different from other commands that it doesn't go through that usual workflow. This is simply gather the options and start a jConsole process. Our future rest service would have nothing to do with it. It needs InternalGfshCommand since it needs reference to the gfsh instance to get the invoker.
[ Full content available at: https://github.com/apache/geode/pull/2962 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao closed pull request #2962: GEODE-5971:
refactor StartJConsoleCommand to use ResultModel
Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.
[ pull request closed by jinmeiliao ]
[ Full content available at: https://github.com/apache/geode/pull/2962 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on issue #2962: GEODE-5971:
refactor StartJConsoleCommand to use ResultModel
Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.
@aditya87
[ Full content available at: https://github.com/apache/geode/pull/2962 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #2962:
GEODE-5971: refactor StartJConsoleCommand to use ResultModel
Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.
for the changes we are making, a junit test should do. An end to end test would need to fire up additional process, hard to control in a test environment.
We should always prefer junit test over integration test and integration over dunit tests. Use dunit tests only when necessary.
[ Full content available at: https://github.com/apache/geode/pull/2962 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] aditya87 commented on pull request #2962:
GEODE-5971: refactor StartJConsoleCommand to use ResultModel
Posted by "aditya87 (GitHub)" <gi...@apache.org>.
👍 Okay. Just wondering.
[ Full content available at: https://github.com/apache/geode/pull/2962 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] aditya87 commented on pull request #2962:
GEODE-5971: refactor StartJConsoleCommand to use ResultModel
Posted by "aditya87 (GitHub)" <gi...@apache.org>.
👍 Okay. Just wondering. I see that the regular "GfshCommand" class also has access to the current Gfsh instance, though. So I'm sure there's something here which I'm not understanding, we can discuss that another time :).
[ Full content available at: https://github.com/apache/geode/pull/2962 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #2962:
GEODE-5971: refactor StartJConsoleCommand to use ResultModel
Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.
Oh, that statement should have a clause: with the same test coverage, junit is preferred than integration test, integration test is preferred than dunit
[ Full content available at: https://github.com/apache/geode/pull/2962 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #2962:
GEODE-5971: refactor StartJConsoleCommand to use ResultModel
Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.
Oh, that statement should have a clause: with the same test coverage, junit is preferred than integration test, integration test is preferred than dunit. Like you don't need a dunit test to test a invalid command option.
[ Full content available at: https://github.com/apache/geode/pull/2962 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] aditya87 commented on pull request #2962:
GEODE-5971: refactor StartJConsoleCommand to use ResultModel
Posted by "aditya87 (GitHub)" <gi...@apache.org>.
Shouldn't this be SingleGfshCommand or GfshCommand?
[ Full content available at: https://github.com/apache/geode/pull/2962 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] aditya87 commented on pull request #2962:
GEODE-5971: refactor StartJConsoleCommand to use ResultModel
Posted by "aditya87 (GitHub)" <gi...@apache.org>.
I am convinced that we don't need a DUnit test for this one.
However, I'm not sure I agree with the general principle of not preferring integration tests? Especially in a distributed system like Geode, I feel that the greater robustness and less mocky nature of integration tests trumps the ease of controlling more mock-heavy "unit" tests. Ideally I'd want both, i.e. integration tests and unit tests.
We can discuss this point outside of this PR.
[ Full content available at: https://github.com/apache/geode/pull/2962 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] aditya87 commented on pull request #2962:
GEODE-5971: refactor StartJConsoleCommand to use ResultModel
Posted by "aditya87 (GitHub)" <gi...@apache.org>.
I am convinced that we don't need a DUnit test for this one.
However, I'm not sure I agree with the general principle of not having integration tests? Especially in a distributed system like Geode, I feel that the greater robustness and less mocky nature of integration tests trumps the ease of controlling more mock-heavy "unit" tests. Ideally I'd want both, i.e. integration tests and unit tests.
We can discuss this point outside of this PR.
[ Full content available at: https://github.com/apache/geode/pull/2962 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] aditya87 commented on pull request #2962:
GEODE-5971: refactor StartJConsoleCommand to use ResultModel
Posted by "aditya87 (GitHub)" <gi...@apache.org>.
I am convinced that we don't need a DUnit test for this one.
However, I'm not sure I agree with the general principle of not having integration tests? Especially in a distributed system like Geode, I feel that the greater robustness and less mock-heavy nature of integration tests trumps the ease of controlling more mock-heavy "unit" tests. Ideally I'd want both, i.e. integration tests and unit tests.
We can discuss this point outside of this PR.
[ Full content available at: https://github.com/apache/geode/pull/2962 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org