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

Already on GitHub? Sign in to your account

Fix slow test. It was formatting a Money object 22 000 times. Reduced to... #215

Merged
merged 1 commit into from Sep 26, 2012

Conversation

Projects
None yet
3 participants
Contributor

realmika commented Sep 26, 2012

... 22 times - now tests run on 0.2 seconds instead of 4.5 seconds!

Mikael Wikman Fix slow test. It was formatting a Money object 22 000 times. Reduced…
… to 22 times - now tests run on 0.2 seconds instead of 4.5 seconds\!
6c88b0d
Owner

semmons99 commented Sep 26, 2012

👍 Ship It! :shipit:

@realmika realmika added a commit that referenced this pull request Sep 26, 2012

@realmika realmika Merge pull request #215 from mikaelwikman/fix_slow_test
Fix slow test. It was formatting a Money object 22 000 times. Reduced to...
8b5dd32

@realmika realmika merged commit 8b5dd32 into RubyMoney:master Sep 26, 2012

1 check passed

default The Travis build passed
Details
Member

kenn commented Sep 26, 2012

I thought the underlaid purpose of the full coverage was to test Float presicion at every corner case in decimal places.

irb> 0.1 + 0.7
 => 0.7999999999999999

http://www.exploringbinary.com/why-0-point-1-does-not-exist-in-floating-point/

Are you sure it's a good idea? At least we should be clear how we chose those specific test cases and, at least, change the test titles - it's no longer brute force. :)

Contributor

realmika commented Sep 26, 2012

Hi @kenn, that's a good point! Do you have an idea of how we could identify a few cases where float point would have been an issue if it wasn't solved as it is today? I would really like to have focused tests for these, rather than going napalm on the whole issue :)

Owner

semmons99 commented Sep 26, 2012

You could try and run the 22000 and find all the errors that it actually fixed. However, since we converted to BigDecimal awhile ago, this should no longer be an issue. At least I hope it isn't.

The other option is split it out this massive run into an acceptance test and have it run with a separate rake spec:acceptance command.

Contributor

realmika commented Sep 26, 2012

If we can confirm that BigDecimal is used everywhere regarding this issue, then we can trust it and settle for a single integration test. Since I'm new to this code it would be best if someone with good experience can make the decision. Up for a vote?

Also, is it possible to re-open this thread somehow since the issue is not resolved?

Owner

semmons99 commented Sep 26, 2012

Can you create a new issue and reference this pull-request there? I will attempt to code spelunk tomorrow or Friday.

Owner

semmons99 commented Oct 1, 2012

@kenn & @mikaelwikman: See my comments on #216, but it appears we have squashed all internal usage of Floats.

@realmika realmika pushed a commit to realmika/money that referenced this pull request Oct 2, 2012

Mikael Wikman Cleaned up the tests according to discussion on pull request #215 and…
… issue #216
132cd4c

@realmika realmika referenced this pull request Oct 2, 2012

Merged

Cleaned up the tests #218

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment