Added tests for NumberParser

[rohancme]

Added tests for the NumberParser class.

I've created a tool that uses Randoop to generate tests and then prunes them using PIT as a measure of quality. So far it's been useful in understanding which classes need more tests in a repo and approaches on how to add Line and Mutation Coverage.

Thought I would start with one of the simpler classes to test and once I get this reviewed I can submit a few more PRs with tests for other classes as well!

[rohancme]

Improvement in Test Coverage (using PIT):

Before:

After:

[mishako]

Would assertEquals work? Same for other tests.

[mishako]

Hi Rohan! Thank you for the contribution. Looks good! I left some comments. They might be a bit nitpicky, but since you're going to continue with this kind of pull requests, I think it's good to do everything properly from the start.

[rohancme]

Thanks for the review @mishako ! Really appreciate it. I'll make the
changes and update soon!

On Fri, Apr 22, 2016, 3:07 PM mishako notifications@github.com wrote:

Hi Rohan! Thank you for the contribution. Looks good! I left some
comments. They might be a bit nitpicky, but since you're going to continue
with this kind of pull requests, I think it's good to do everything
properly from the start.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#296 (comment)

[rohancme]

@mishako I've made the suggested changes. I'd appreciate you taking a quick look!

[mishako]

@rchakra3 You've included commits from master in your branch.

[rohancme]

@mishako Sorry I've fixed that!

[mishako]

@rchakra3 Nice! Do you want to squash commits into one before we merge it?

[rohancme]

@mishako Done! :)

[mishako]

@rchakra3 Ah, sorry, I missed more things:

• The first argument of assertEquals(a, b) is expected value, not the other way around.
• The first argument of assertEquals(a, b, c) is message (see two bottom tests).

[rohancme]

@mishako I've made the changes. Do the messages look fine?

[mishako]

@rchakra3 The messages look fine, but they are pretty much what assertEquals has by default, so do you really need them?

[rohancme]

@mishako That's true. I assumed you expected a custom message as per your second point above:
"The first argument of assertEquals(a, b, c) is message (see two bottom tests)."
since assertEquals(a, b, c) with a as the message seems to be deprecated

Did I misunderstand you?

[mishako]

@rchakra3 No, it's me, sorry. You were correctly using delta as the third argument, I didn't know about it and assumed you made a mistake. Sorry! :)

[rohancme]

@mishako Not a problem! :)

I think I've got all the changes in now!