Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

removed unmotivated throwing of UnsupportedEncodingException #75

Merged

Conversation

neshanjo
Copy link
Contributor

@neshanjo neshanjo commented Sep 5, 2017

With this small fix, the API is easier to use, since the consumer does not have to handle the UnsupportedEncodingException which is neither motivated nor documented in javadoc.

@lbalmaceda lbalmaceda modified the milestones: 1.2.0, v1-Next Sep 6, 2017
@lbalmaceda
Copy link
Contributor

Thanks! Yeah, I know the encoding should be supported that's why I had that exception anyway, but it's better to make it runtime. 👍 Coverage check won't let me merge it without the corresponding tests. A simple "shouldThrowWhenQueryEncodingNotSupported" test on each class would suffice. You'll need to add a @VisibileForTesting package private method in the QueryFilter class to urlEncode(String value), which will return the output of URLEncoder.encode(value, "UTF-8"); but for a single test on each class we will mock that to throw an exception when called with that value and encoding. Tests will look like this:

@Rule
public ExpectedException exception = ExpectedException.none()

@Test
public void shouldThrow...() throws Exception {
  exception.expect(IllegalStateException)
  exception.expectMessage("the exception message goes here")
  String value = "my=test value"
  QueryFilter filter = Mockito.spy(new QueryFilter())
  Mockito.doThrow(UnsupportedEncodingException).when(filter.urlEncode(value, "UTF-8"))
  filter.withQuery(value)
}

I'm sure you'll find similar tests for other classes. Please repeat that for each class and the coverage will raise again.
Cheers

@neshanjo neshanjo force-pushed the removed-unsupported-enconding-exception branch from d7528e5 to 06fb87e Compare September 8, 2017 06:35
@neshanjo neshanjo force-pushed the removed-unsupported-enconding-exception branch from 06fb87e to 77f936d Compare September 8, 2017 06:41
@neshanjo
Copy link
Contributor Author

neshanjo commented Sep 8, 2017

Done. Couldn't find @VisibileForTesting somewhere in the project, so I've omitted it.

Copy link
Contributor

@lbalmaceda lbalmaceda left a comment

Choose a reason for hiding this comment

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

yeah.. that's an android annotation, my bad 😛 thanks again! 👏

@lbalmaceda lbalmaceda merged commit faf4adf into auth0:master Sep 8, 2017
@lbalmaceda lbalmaceda modified the milestones: v1-Next, 1.3.0 Sep 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants