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

Add test file for the 'modf' routine in rakudo #742

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tbrowder
Copy link
Member

@tbrowder tbrowder commented Jun 28, 2021

See rakudo PR rakudo/rakudo#4434.

@tbrowder tbrowder requested a review from vrurg June 28, 2021 02:29
@tbrowder
Copy link
Member Author

tbrowder commented Jul 2, 2021

Note there will be more tests added.

@tbrowder tbrowder marked this pull request as draft July 2, 2021 12:01
Copy link
Contributor

@vrurg vrurg left a comment

Choose a reason for hiding this comment

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

I wonder if there're any chance for errors on user side. I'd be glad to see testing for incorrect user input. How modf must react on *? Inf, NaN?

Also, I didn't follow the original discussion about modf, but why $val is in stringified form only? This way you test the Cool candidate only. This is right, in the light of the proposed PR you kinda test every multi-candidate then. But testing must not rely on particular implementation.

And then, tests for positives must be explicitl, i.e. included into the table. When you use abs you turn $val into a Numeric, thus testing different multi-candidates compared to the tests for negative. It effectively means that you kind of testing two things at once: multi-dispatch and the type of argument.

S32-num/modf.t Show resolved Hide resolved
@tbrowder
Copy link
Member Author

tbrowder commented Jul 4, 2021

Note I haved closed the associated Rakudo PR #4434 which I will be rewriting and resubmitting as a new PR. This roast PR will continue to be used by the new Rakudo PR (which I will not push until I am happy with it).

@tbrowder
Copy link
Member Author

tbrowder commented Jul 6, 2021

@vrurg, I've been doing some experiments and would appreciate your thoughts on this:

  • use only one set of desired test values, all as strings with a leading '-' sign:

    my $x-neg-str = '-0.e5';

  • remove the leading minus signs from the strings to get positive versions:

    my $x-pos-str = $x-neg-str.substr: 1;

  • convert each to a RatStr or NumStr by using <<>> quotes:

    my $x-neg-allomorph = <<$x-neg-str>>;
    my $x-pos-allomorph = <<$x-pos-str>>;

  • convert each allomorph to the appropriate number type:

    my ($x-neg-rat, $x-neg-num, $x-pos-rat, $x-pod-num);
    if $x-neg-allomorph ~~ Rat {
    $x-neg-rat = $x-neg-allomorph.Rat;
    $x-pos-rat = $x-pos-allomorph.Rat;
    }
    elsif $x-neg-allomorph ~~ Num {
    $x-neg-num = $x-neg-allomorph.Num;
    $x-pos-num = $x-pos-allomorph.Num;
    }
    else {
    die "FATAL: Unexpected input '$x-neg-str' of type '{$x-neg-str.^name}'";
    }

Then I think we would have all the number type possibilities to test, as applicable.

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.

2 participants