Skip to content

Conversation

@jdeppe-pivotal
Copy link
Contributor

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?

Note:

Please ensure that once the PR is submitted, check Concourse 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.

@jdeppe-pivotal jdeppe-pivotal marked this pull request as ready for review February 9, 2021 19:53
Copy link
Contributor

@nonbinaryprogrammer nonbinaryprogrammer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good but can you add a test for the right number of command line options?

@jdeppe-pivotal
Copy link
Contributor Author

looks good but can you add a test for the right number of command line options?

I think these are the tests you are looking for:

@Test
public void givenKeyNotProvided_returnsWrongNumberOfArgumentsError() {
assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.GETRANGE))
.hasMessageContaining("ERR wrong number of arguments for 'getrange' command");
}
@Test
public void givenStartNotProvided_returnsWrongNumberOfArgumentsError() {
assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.GETRANGE, "key"))
.hasMessageContaining("ERR wrong number of arguments for 'getrange' command");
}
@Test
public void givenEndNotProvided_returnsWrongNumberOfArgumentsError() {
assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.GETRANGE, "key", "1"))
.hasMessageContaining("ERR wrong number of arguments for 'getrange' command");
}
@Test
public void givenMoreThanFourArgumentsProvided_returnsWrongNumberOfArgumentsError() {
assertThatThrownBy(
() -> jedis.sendCommand(Protocol.Command.GETRANGE, "key", "1", "5", "extraArg"))
.hasMessageContaining("ERR wrong number of arguments for 'getrange' command");
}

Copy link
Member

@sabbey37 sabbey37 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just thinking, we don't have a test verifying the error message when the start/end integers are more than the min/max value. The min in Redis is -9,223,372,036,854,775,808 and max is 9,223,372,036,854,775,807

@jdeppe-pivotal jdeppe-pivotal merged commit f8413bc into apache:develop Feb 16, 2021
jhutchison pushed a commit to jhutchison/geode that referenced this pull request Apr 15, 2021
nabarunnag pushed a commit that referenced this pull request Apr 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants