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

Big decimal range #69

Merged
merged 3 commits into from Dec 25, 2014
Merged

Big decimal range #69

merged 3 commits into from Dec 25, 2014

Conversation

csrcordeiro
Copy link
Contributor

Hi Fixture Factory community,

This pull request is regarding issue #61.
To Implement this, I had to change both the BigDecimal and BigInteger features but I believe it will be ok since I was able to write tests for both cases. I also noticed a little typo in README tutorial so I fixed it.

There are tests failing in master branch but I couldn't figure out what it was about so I didn't fixed it, the tests are:

  • shouldCreateImmutableObjectUsingCorrectPartialConstructor
  • shouldCreateImmutableObjectUsingAnotherPartialConstructor


assertNotNull("Generated BigDecimal must not be null", bigDecimal);
assertTrue("Generated value must be a BigDecimal", bigDecimal instanceof BigDecimal);
assertTrue(new BigDecimal("1").compareTo(bigDecimal) != 1);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe here should be better to check if the generated value is in the range instead of checking if it's not the limit values. Something like:

assertTrue(new BigDecimal("1").compareTo(bigDecimal) >= 1);
assertTrue(new BigDecimal("1000").compareTo(bigDecimal) <= 1000);

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback.

Yeah I agree, reading BigDecimal comparison is really annoying. But the problem is that the compareTo method will only return (-1, 0, 1) for less than, equals to, greater than the other value.

But I'll come up with some clever idea to make it a little more readable.

Copy link
Member

Choose a reason for hiding this comment

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

@csrcordeiro actually is possible to compare exactly the same as with operators, check the Javadoc here: http://docs.oracle.com/javase/7/docs/api/java/math/BigDecimal.html#compareTo(java.math.BigDecimal)

You could change your asserts to be like this:

assertTrue(bigDecimal.compareTo(new BigDecimal("1")) >= 0);
assertTrue(bigDecimal.compareTo(new BigDecimal("1000")) <= 0);

The above code is the same as:

assertTrue(number >= 1);
assertTrue(number <= 1000);

@nykolaslima
Copy link
Member

@csrcordeiro Travis CI ran all tests and nothing failed. I also ran the tests on my machine and the tests you mentioned worked:

  • shouldCreateImmutableObjectUsingCorrectPartialConstructor
  • shouldCreateImmutableObjectUsingAnotherPartialConstructor

Maybe it's a bug? What's your environment? Java version etc.

I only added two little comments about the tests.

@csrcordeiro
Copy link
Contributor Author

@nykolaslima Here are my environment settings:

  • Maven (3.2.3)
  • Java (1.8.0_25)
  • Debian operating system.

The cause is a little bit odd, it seems to be in bytecode level so might be a OS issue:

net.vidageek.mirror.exception.MirrorException: Could not find constructor with args [class br.com.six2six.fixturefactory.model.Immutable] on class br.com.six2six.fixturefactory.model.Immutable$ImmutableInner

I'll try to take a look at the problem a little further.

@nykolaslima
Copy link
Member

@csrcordeiro about this MirrorException, I believe we can open a new issue to investigate it. What do you think?

@csrcordeiro
Copy link
Contributor Author

Ok, I'll just fix the tests assertions.

nykolaslima added a commit that referenced this pull request Dec 25, 2014
@nykolaslima nykolaslima merged commit c53e753 into six2six:master Dec 25, 2014
@nykolaslima
Copy link
Member

Thanks @csrcordeiro

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.

None yet

2 participants