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

transpiled precision testing :) #2397

Closed
wants to merge 16 commits into from
Closed

Conversation

frosty00
Copy link
Member

@frosty00 frosty00 commented Mar 29, 2018

This commit adds precision test transpilling to python and php from the javascript "master" test file. This should be useful in ensuring the behaviour of the precision methods are the same in all languages. I already noticed some slight inconsistencies with the php decimalToPrecision:

<?php

include_once 'ccxt.php';
use ccxt\Exchange;
use const ccxt\AFTER_DOT;
use const ccxt\NO_PADDING;
use const ccxt\PAD_WITH_ZERO;
use const ccxt\ROUND;
use const ccxt\SIGNIFICANT_DIGITS;
use const ccxt\TRUNCATE;
$desc = 'decimalToPrecision failed with ';

print Exchange::decimalToPrecision('12', ROUND, 5, SIGNIFICANT_DIGITS, PAD_WITH_ZERO);
print "\n";
print Exchange::decimalToPrecision('12.3', ROUND, 5, SIGNIFICANT_DIGITS, PAD_WITH_ZERO);

> 12.000
> 12.3

And I'm sure many more little inconsitencies that we haven't spotted exist in other languages.

This was my first attempt at transpilling using javascript so feel free to make any changes if you think you can make the code better. Oh, and I fixed the indentation in transpile.js - for some reason half the file was indented with an extra four spaces.

Also this makes most of the tests in ExchangeTest.php redundant, and it means that './js/test/base/functions/test.number.js' is now more sensitive to syntax / formatting changes.

Finally, you may want to use a different difftool to github to show my changes to transpile.js better.

@frosty00
Copy link
Member Author

Not sure why the appveyor build failed, test base is working on my end and I haven't made any changes to the base code.

@kroitor kroitor self-assigned this Mar 29, 2018
@kornrunner
Copy link
Contributor

PHP tests were added within #2171, specifically within https://github.com/ccxt/ccxt/pull/2171/files#diff-7e120f885bf660540c3075e0bbff1d3a (classic phpunit tests)

I did have a disclaimer that it was just a Python port to PHP, and it possibly doesn't cover all possible cases - if any updates are needed I can offer help.

@kroitor
Copy link
Member

kroitor commented Mar 29, 2018

I did have a disclaimer that it was just a Python port to PHP, and it possibly doesn't cover all possible cases - if any updates are needed I can offer help.

We just decided to unify the code of the precision test to make it transpileable and keep it in synch among different languages. So, no worries about some cases, we will accumulate them all in the process ) Thanks as always for your help )

@frosty00
Copy link
Member Author

@kornrunner I just learned a little php and it's horrifying. I don't blame you for the behaviour as there are probably just as many (if not more) bugs waiting to found in python and javascript. Rigorous test cases will help iron them out.

@kroitor
Copy link
Member

kroitor commented Apr 5, 2018

Merging this one next )

@frosty00
Copy link
Member Author

@kroitor please take a look at https://github.com/ccxt/ccxt/pull/2397/files#diff-c6d18b6c7cb481416538ede931ab2766 and let me know when we can merge

@kroitor
Copy link
Member

kroitor commented Apr 30, 2018

@frosty00 I've pulled your changes with my current version like a week ago (there were some restructuring edits), but didn't have a chance to test and finish the build since then yet. I'm still determined to do that in 24-48 hrs and will let you know, of course ) Thx!

@frosty00
Copy link
Member Author

Awesome I just removed the indentation changes in transpile.js so you could see it better. Will also try get the builds to complete now.

@kroitor
Copy link
Member

kroitor commented Apr 30, 2018

@frosty00 yep, got it ) one thing I wanted to ask you: because I'm already deeply in the process, please don't do too big modifications to transpile.js just yet (in the next hours before I merge this PR), will ping you when I'm done with it )

@frosty00
Copy link
Member Author

frosty00 commented Apr 30, 2018

Yep wasn't planning on, there haven't been any changes except me keeping the original indentation.

Have you already starting building on my original branch?

Oh ok nevermind, keep your version transpile.js and only merge the other files.

@kroitor
Copy link
Member

kroitor commented Apr 30, 2018

Have you already starting building on my original branch?

Yep, I've cleaned it up a bit and generalized it further, hope you'll like it )

@frosty00
Copy link
Member Author

frosty00 commented Apr 30, 2018

Sweet, thanks @kroitor - I think the builds are passing but I can't tell because they seem to have hit errors unrelated to my commits. Once this is merged I we can merge the bitfinex decimalToPrecision and I can start using bitfinex and ethfinex again.

@frosty00
Copy link
Member Author

Oops it seems I've got the commits in this PR mixed up with my pythonJSON commits. Perhaps ignore those as I will do that on another branch and just push from your local copy instead. Cheers again,

@kroitor
Copy link
Member

kroitor commented Apr 30, 2018

@frosty00 no worries, will sort it out )

@kroitor
Copy link
Member

kroitor commented May 2, 2018

Working on this PR right now, hope to get through this soon... This is going to be a big update, will have to bump the minor version as well. Will keep you informed on my progress, should not take too much from there.

@frosty00
Copy link
Member Author

frosty00 commented May 2, 2018

Awesome, can't wait to see the changes to transpile.js, although it'll probably be some regex that is way to complex for me to understand :P

@frosty00
Copy link
Member Author

Ok let me revert the pythonJSON commits then you can commit this

kroitor added a commit that referenced this pull request May 31, 2018
@kroitor kroitor closed this in f3e1a71 May 31, 2018
@kroitor
Copy link
Member

kroitor commented May 31, 2018

Ok, I've finally merged it, dunno why GitHub shows it as closed, as I've actually added most of the changes from this PR to the master.

@kornrunner i'm having trouble with the following testcases:

mbp:ccxt igorkroitor$ php -f php/test/decimal_to_precision.php

Warning: assert(): assert(decimal_to_precision('0', TRUNCATE, 0, DECIMAL_PLACES) === '0') failed in /Users/igorkroitor/ccxt/php/test/decimal_to_precision.php on line 55

Warning: assert(): assert(decimal_to_precision('0', TRUNCATE, 0, SIGNIFICANT_DIGITS) === '0') failed in /Users/igorkroitor/ccxt/php/test/decimal_to_precision.php on line 79

Warning: assert(): assert(decimal_to_precision('123', TRUNCATE, 0) === '123') failed in /Users/igorkroitor/ccxt/php/test/decimal_to_precision.php on line 137

Warning: assert(): assert(decimal_to_precision('0.', TRUNCATE, 0) === '0') failed in /Users/igorkroitor/ccxt/php/test/decimal_to_precision.php on line 145

Warning: assert(): assert(decimal_to_precision('1.45', ROUND, 1, DECIMAL_PLACES) === '1.5') failed in /Users/igorkroitor/ccxt/php/test/decimal_to_precision.php on line 152
mbp:ccxt igorkroitor$

If you could help with solving those – that would be really awesome!

Anyway, thanks to you both for your help on this project, @kornrunner , @frosty00!

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

3 participants