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

Avoid extra number conversions #341

Merged
merged 3 commits into from
May 7, 2020
Merged

Avoid extra number conversions #341

merged 3 commits into from
May 7, 2020

Conversation

kappa
Copy link
Contributor

@kappa kappa commented Apr 22, 2020

We don't need BigFloat magic here as Grains.pm will always return bigints.

This should fix #239 and #240.

We don't need BigFloat magic here as Grains.pm will always return bigints.

This should fix exercism#239 and exercism#240.
@kappa
Copy link
Contributor Author

kappa commented Apr 22, 2020

Ok, I am reading #240 comments and see that this is not the correct way to fix tests for exercises. I'll fix the pull request.

@kappa
Copy link
Contributor Author

kappa commented Apr 22, 2020

Travis failure fixed in #343.

@m-dango
Copy link
Member

m-dango commented May 3, 2020

I'm not too sure what the problem we're solving is here. If bigint/bignum is removed from Grains.pm, this is the result of the test:

exercises/grains/.meta/solutions/grains.t .. 
# Seeded srand with seed '20200503' from local date.
1..12
ok 1 - Imported symbols: grains_on_square, total_grains
ok 2 - square no. 1
ok 3 - square no. 2
ok 4 - square no. 3
ok 5 - square no. 4
ok 6 - square no. 16
ok 7 - square no. 32
not ok 8 - square no. 64

# Failed test 'square no. 64'
# at exercises/grains/.meta/solutions/grains.t line 17.
# +---------------------+----+---------------------+-----+
# | GOT                 | OP | CHECK               | LNs |
# +---------------------+----+---------------------+-----+
# | 9223372036854779904 | == | 9223372036854775808 | 29  |
# +---------------------+----+---------------------+-----+
ok 9 - square 0 raises an exception
ok 10 - negative square raises an exception
ok 11 - square greater than 64 raises an exception
not ok 12 - returns the total number of grains on the board

# Failed test 'returns the total number of grains on the board'
# at exercises/grains/.meta/solutions/grains.t line 17.
# +----------------------+----+----------------------+-----+
# | GOT                  | OP | CHECK                | LNs |
# +----------------------+----+----------------------+-----+
# | 1.84467440737096e+19 | == | 18446744073709551615 | 36  |
# +----------------------+----+----------------------+-----+
Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/12 subtests 

Test Summary Report
-------------------
exercises/grains/.meta/solutions/grains.t (Wstat: 512 Tests: 12 Failed: 2)
  Failed tests:  8, 12
  Non-zero exit status: 2
Files=1, Tests=12,  0 wallclock secs ( 0.02 usr  0.00 sys +  0.10 cusr  0.01 csys =  0.13 CPU)
Result: FAIL

Also, these changes result in doing a string comparison rather than a numeric one. Do you have an example/link to a solution where issues have come up for this?

@kappa
Copy link
Contributor Author

kappa commented May 3, 2020

The problem to solve is #239. There are examples of problematic solutions that pass the tests there. That issue is old and it had been fixed at one point but then regressed and is relevant for the current master.

My change removes BigFloat from the test code and relies on the Grains.pm having "use bigint" and always returning bigints -- and now I see the problem, this is not present in the stub which I incorrectly assumed.

@m-dango
Copy link
Member

m-dango commented May 7, 2020

Ah yes I see what's happened.

use bigint was intentionally excluded from the stub. The exercise is quite straightforward without it, and I sense that leaving it in would have people miss that it is there and what it is for.

@m-dango m-dango merged commit efc52be into exercism:master May 7, 2020
@m-dango
Copy link
Member

m-dango commented May 7, 2020

Thanks @kappa!

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.

Grains test doesn't handle bugs caused by **
2 participants