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

Improve tests by removing EVAL and refactoring #659

Closed
wants to merge 21 commits into from
Closed

Improve tests by removing EVAL and refactoring #659

wants to merge 21 commits into from

Conversation

tbrowder
Copy link
Member

Improve tests by removing EVAL and refactoring

This commit:

  • Removes subroutines used by:

    S26-documentation/12-non-breaking-space.t
    S32-str/space-chars.t

    to a new module:

    packages/Test-Helpers/lib/Test/Misc.pm6

  • Changes the EVALed code in test 12-non-breaking-space.t to a normal test

Files affected:

S26-documentation/12-non-breaking-space.t
S32-str/space-chars.t
packages/Test-Helpers/lib/Test/Misc.pm6

vrurg and others added 20 commits July 26, 2020 22:49
Do everything on a in-memory object. After all, the file content is
`slurp`ed into one anyway.
The main tests are fudged with `skip` with which it is easy to miss when
dispatching gets fixed. The canary test is fudged with `todo` and will
result in a notification when it's about time to unfudge everything.
The test is fudged as a `todo`
And add it to spectest.data.

Two tests are fudged for JVM backend.
`EVAL` throws when compilation fails. A `try` is required to contain the
exception and pass control over to the subsequent `skip` statement.

TODOing the tests would be much more preferable but I don't see a quick
solution to implement it.
Added two canary tests for `&method` and `::=`.
Fudged with `eval` and added a canary test.
Passing on both moar and jvm backends
The test was erroneously considering `EVAL` to be the same scope as
surrounding block. Whereas in fact it's a scope on its own.
Wonder if the TODO'ed test has to be the way it is expected to be. The
current state of things seems rather logical to me.
The test seems to be extremely outdated with traces of early Perl6
design patters. Besides, the test now passes as whole.
This commit:

+ Removes subroutines used by:

    S26-documentation/12-non-breaking-space.t
    S32-str/space-chars.t

  to a new module:

    packages/Test-Helpers/lib/Test/Misc.pm6

+ Changes the EVALed code in test 12-non-breaking-space.t to a normal test

Files affected:

    S26-documentation/12-non-breaking-space.t
    S32-str/space-chars.t
    packages/Test-Helpers/lib/Test/Misc.pm6
@vrurg
Copy link
Contributor

vrurg commented Jul 29, 2020

Seems like the branches diverged. Could you please try rebasing onto the current roast_657?

@tbrowder
Copy link
Member Author

Seems like the branches diverged. Could you please try rebasing onto the current roast_657?

Will do.

Raku Language  ᠎             is the best! | Raku   Language  ᠎             is the best!
Raku⁠Language  ᠎             is the best! | Raku ⁠ Language  ᠎             is the best!
RakuLanguage  ᠎             is the best! | Raku  Language  ᠎             is the best!
=end table
Copy link
Contributor

Choose a reason for hiding this comment

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

I think removal of the pod-generation part is unfortunate. Hard-coding of whitespaces directly into the test code is dangerous because a "malicious" editor could "normalize" some of them. The best solution, to my view, is to use the approach I've shown in #657 (comment) where pod code is EVALed but then the Pod::Block object is pulled from inside of the EVAL scope.


my $n = 4;
my $SPACE = ' ';
my $r = $=pod[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

With the previous comment in mind, this line would then be replaced with my $r = get-pod; and the rest of the test would remain unchanged. So, we could get the best of two variants: list of chars with codes and names from the initial EVALed version, and easy to work with test code outside of any EVAL.

Copy link
Member Author

@tbrowder tbrowder Jul 29, 2020

Choose a reason for hiding this comment

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

I'll take another look and see what I can come up with.

Copy link
Contributor

Choose a reason for hiding this comment

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

The point is not in making it smaller but to have only the pod section evaled. The biggest problem with the initial approach is the difficulty to analyze and modify code spread across several ~= statements.

Simply put, from the original test I'm interested in preserving lines 15 to 79.

Copy link
Member Author

@tbrowder tbrowder Jul 29, 2020

Choose a reason for hiding this comment

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

I can use your example but I can't see how to generate the code internally in the same comp unit without more EVALing. I can generate it fine from another program.

Suggestions? If you know how to do it please show me a short example.

Copy link
Contributor

@vrurg vrurg Jul 29, 2020

Choose a reason for hiding this comment

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

If I got the misunderstanding correctly, I don't mean to get rid of EVAL altogether. I only want to minimize its use. Thus, EVALing the generated code, the way I eval a piece of pod with a primitive sub in my example, is totally ok.

I mean the separation I'm trying to reach is to make the test code, which is to be read and understood by a human, available directly in one clean piece; and only generate and EVAL the pod block as it is just test input data and barely would be modified or analyzed by a person.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I understand what you want, but I don't see how to do it with a single script. Let me try a new tack and see your reaction. I should get it here later today.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me repeat the example code here:

sub get-pod {
    my $pod;
    my sub pull-pod($p) {
        $pod = $p;
    }
    EVAL q:to/TEST/;
        =begin pod
        Pod text goes here...
        =end pod
        pull-pod($=pod[0]);
        TEST
        
    $pod
}
my $pod = get-pod;
isa-ok $pod, Pod::Block, "fine";

This approximately what I'd like to achieve.

Copy link
Member Author

@tbrowder tbrowder Jul 31, 2020

Choose a reason for hiding this comment

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

I looked again at the situation this morning but I won't start work until you're happy with my proposal below.

First note that I found at least eight test generation programs in roast and that is what I intend to add to the S26 directory. Essentially, I will modify what I did to generate the current test by adding my Raku program and incorporate your excellent method to minimize the EVALed code.

PROPOSAL

  • Create two code templates:

    • test-file.raku.top
    • test-file.raku.bottom
  • Create the Raku test generator to:

    • write the top part to the test file
    • generate and append the EVALed test code to the test file
    • append the bottom part to the test file (I will add some code to all parts as a check on planned tests such how many table rows are expected and how many were actually found)
  • Create a README for the directory to explain the situation

  • Put all three non-test parts in a new tools subdirectory

SUMMARY

I believe my approach helps by:

  • easing creation and debugging of the main top and bottom of the test file
  • clearly show the table generation (the only EVALed code) and characters used
  • explaning the approsch for future devs
  • declutters the main S26 directory by moving the generation code to a subdirectory

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think two extra files and manipulations with them would greatly improve the test.

I probably failed to explain my idea clearly after all. Please, review 4fffc73 and check if it does what it should. I have grouped the tests into subtests. And added two more tests per row (per subtest). So, now, instead of just outputting the converted lines of ws codes it compares them.

@vrurg
Copy link
Contributor

vrurg commented Jul 29, 2020

Apart from me wanting to preserve the pod generation code, things are looking so much better this way! Thank you!

@Altai-man
Copy link
Member

Make sure those changes apply well to 6.c-errata and 6.d-errata.
We usually cherry-pick some sane behavior-related changes to spec, but I wonder if massive changes for the sake of code (not spec sanity itself) are necessary.

@vrurg
Copy link
Contributor

vrurg commented Jul 30, 2020

@Altai-man the test is not even in spectest.data yet. I don't see a problem here. Otherwise we must think about the manageability of test code because we must remember about those coming into these waters after us.

@vrurg
Copy link
Contributor

vrurg commented Aug 1, 2020

I think it makes sense to close this PR after 4fffc73.

@vrurg vrurg closed this Aug 1, 2020
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.

3 participants