Fix for GEODE-4771: Defaults in ConfigurePDXCommand #1574
Conversation
jujoramos
force-pushed
the
feature/GEODE-4771
branch
from
f9c4802
to
b5cccce
Compare
- Refactored `ConfigurePDXCommand` to allow unit testing. - Added a custom `Interceptor` to validate command input. - Added unit and integration tests for `ConfigurePDXCommand`. - Fixed help strings for `auto-serializable-classes` and `portable-auto-serializable-classes`. - Fixed `ConfigurePDXCommand` to set `check-portability=false` when `--auto-serializable-classes` is used.
jujoramos
force-pushed
the
feature/GEODE-4771
branch
from
b5cccce
to
a1cb166
Compare
|
|
||
| @Override | ||
| public Result preExecution(GfshParseResult parseResult) { | ||
| Object portableClassesPatterns = |
There was a problem hiding this comment.
you can do the casting here: String[] portableClassesPatterns = (String[])xxxx;
| result = ResultBuilder.buildResult(ird); | ||
| persistClusterConfiguration(result, | ||
| () -> getSharedConfiguration().addXmlEntity(xmlEntity, null)); | ||
|
|
||
| } catch (Exception e) { |
There was a problem hiding this comment.
you can get rid of this try/catch block here. The executor of the command would do that already.
|
|
||
| @Test | ||
| @Ignore("See https://issues.apache.org/jira/browse/GEODE-4794") | ||
| public void commandShouldSucceedWhenUsingDefaults() { |
There was a problem hiding this comment.
what would be the default when you issue this command with no options? Or should we add validation to the command that you should have specified at least one option?
There was a problem hiding this comment.
@jinmeiliao: good catch. I've added the respective assertions in my last commit. The @Ignore should be removed as part of the fix for GEODE-4794.
| verify(command, times(1)).createReflectionBasedAutoSerializer(false, patterns); | ||
| } | ||
|
|
||
| static class CustomGfshParserRule extends GfshParserRule { |
There was a problem hiding this comment.
instead of adding this class, you can just add a method in CommandResultAssert to expose the commandResult there. We didn't do that because we hadn't had a need to, but now that you need it to do custom assertion. We should just add the method getCommandResult.
There was a problem hiding this comment.
@jinmeiliao: the CommandResultAssert class already exposed the method getCommandResult() when I wrote the test, I needed something different.
I basically wanted a PDXCommandResultAssert (just changed the name in my last commit so it makes more sense), internal to the test class, so I can assert different functional things related to the PDX configuration command in just one invocation (hasPersistenceConfigured, hasDefaultsConfigured, etc.) without having to repeat the assertions in every single test method; it looks more readable and natural.
The method GfshParserRule.executeAndAssertThat() returns an instance of CommandResultAssert but I want it to return an instance of my PDXCommandResultAssert assertion, that's why I extended the class in the first place.
Do you see any complications with this approach?.
Cheers.
ClusterConfigurationService was changed to InternalClusterConfigurationService after the original PR (apache#1574) was created.
ClusterConfigurationService was changed to InternalClusterConfigurationService after the original PR (#1574) was created.
|
This was pulled in with my commit 3fa3074 |
ConfigurePDXCommandto allow unit testing.Interceptorto validate command input.ConfigurePDXCommand.auto-serializable-classesandportable-auto-serializable-classes.ConfigurePDXCommandto setcheck-portability=falsewhen--auto-serializable-classesis used.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 buildrun 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?
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.