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

Tests missing from spectest.data: worth adding them? #657

Open
vrurg opened this issue Jul 25, 2020 · 17 comments
Open

Tests missing from spectest.data: worth adding them? #657

vrurg opened this issue Jul 25, 2020 · 17 comments

Comments

@vrurg
Copy link
Contributor

vrurg commented Jul 25, 2020

After a recent fix for Rakudo's update-passing-test-data.pl script I've done a test run. Here is the output:

Key:
[S  ]   = some tests passed
[ P ]   = plan ok (ran all tests)
[  A]   = all passed
      ( passed / planned or ran )
==================================
[   ] (  0/0  ) t/spec/S13-overloading/multiple-signatures.t
[   ] (  0/0  ) t/spec/S13-overloading/fallbacks-deep.t
[   ] (  0/0  ) t/spec/S16-unfiled/getpeername.t
[ PA] (  0/21 ) t/spec/S17-procasync/windows-arg-quoting.t
[SPA] (  9/9  ) t/spec/S26-documentation/12-non-breaking-space.t
[SPA] ( 30/30 ) t/spec/S26-documentation/13-space-chars.t
[SPA] (  3/3  ) t/spec/S26-documentation/11-non-breaking-space.t
[   ] (  0/0  ) t/spec/S01-perl-5-integration/modify_inside_p5.t
[   ] (  0/0  ) t/spec/S01-perl-5-integration/modify_inside_p5_p6.t
[SPA] (  4/4  ) t/spec/S32-list/toggle.t
[   ] (  0/0  ) t/spec/S12-traits/basic.t
[   ] (  0/0  ) t/spec/S12-traits/parameterized.t
[   ] (  0/0  ) t/spec/S03-operators/shortcuts.t
[   ] (  0/0  ) t/spec/S19-command-line-options/01-dash-uppercase-i.t
[   ] (  0/0  ) t/spec/S12-class/open_closed.t
[   ] (  0/0  ) t/spec/S13-syntax/aliasing.t
[   ] (  0/0  ) t/spec/S24-testing/2-force_todo.t
[S  ] (  2/2  ) t/spec/S24-testing/6-done_testing.t
[S  ] (  1/1  ) t/spec/S24-testing/1-basic.t
[SPA] (  9/9  ) t/spec/S17-supply/Seq.t
[SPA] (  4/4  ) t/spec/S17-supply/watch-path.t
[SP ] (  4/10 ) t/spec/S32-io/child-secure.t
[   ] (  0/0  ) t/spec/S10-packages/export.t
[   ] (  0/2  ) t/spec/S10-packages/require-and-use--dead-file.t
[SPA] (  9/9  ) t/spec/S10-packages/nested-use.t
[SP ] ( 22/23 ) t/spec/S10-packages/scope.t
[S  ] (  2/51 ) t/spec/S05-modifier/exhaustive.t
[   ] (  0/0  ) t/spec/S05-modifier/ratchet.t
[   ] (  0/0  ) t/spec/t/fudgeandrun.t
[SPA] ( 10/10 ) t/spec/t/test-util/01-is-eqv.t
[   ] (  0/0  ) t/spec/S14-traits/package.t
[   ] (  0/0  ) t/spec/S14-traits/variables.t
[   ] (  0/0  ) t/spec/S05-capture/external-aliasing.t
[   ] (  0/0  ) t/spec/S05-capture/hash.t
[SP ] (  6/8  ) t/spec/S11-modules/gh2979.t
[   ] (  0/0  ) t/spec/S11-modules/re-export.t
[SPA] (  9/9  ) t/spec/S11-modules/versioning.t
[   ] (  0/0  ) t/spec/S11-modules/use-perl-6.t
[   ] (  0/0  ) t/spec/S05-nonstrings/basic.t
[S  ] (  1/2  ) t/spec/S12-attributes/augment-and-initialization.t
[   ] (  0/0  ) t/spec/S12-attributes/trusts.t
[   ] (  0/0  ) t/spec/S14-roles/attributes-6e.t
[   ] (  0/0  ) t/spec/S14-roles/versioning.t
[SPA] ( 41/41 ) t/spec/S05-mass/properties-contributory.t
[   ] (  0/0  ) t/spec/S06-signature/multiple-signatures.t
[   ] (  0/0  ) t/spec/S06-signature/slurpy-blocks.t
[   ] (  0/0  ) t/spec/S06-macros/returning-string.t
[   ] (  0/0  ) t/spec/S06-macros/returning-closure.t
[   ] (  0/0  ) t/spec/S06-macros/postfix.t
[   ] (  0/0  ) t/spec/S04-phasers/interpolate.t
[S  ] (  1/1  ) t/spec/S04-phasers/exit-in-check.t
[   ] (  0/0  ) t/spec/S04-statements/goto.t
[   ] (  0/0  ) t/spec/S04-statements/leave.t
[   ] (  0/0  ) t/spec/S04-statements/lazy.t
[   ] (  0/0  ) t/spec/S32-temporal/time.t
[SP ] ( 1752/2204) t/spec/S32-str/val.t
[   ] (  0/0  ) t/spec/S06-advanced/return_function.t
[   ] (  0/0  ) t/spec/S06-advanced/dispatching.t
[   ] (  0/0  ) t/spec/S06-advanced/caller.t
[SPA] ( 124/124) t/spec/S03-sequence/exhaustive.t

I wonder if we should consider adding the passing tests to spectest.data. Even partially passing ones perhaps can be considered worth fudging and adding.

Specifically, @JJ – maybe you could say something about S26-documentation ones?

Otherwise I'm looking for any possible help with this.

Will be adding more comments as soon as I will proceed through individual tests.

@JJ
Copy link
Contributor

JJ commented Jul 25, 2020

Ok, will check

@JJ
Copy link
Contributor

JJ commented Jul 25, 2020 via email

@vrurg
Copy link
Contributor Author

vrurg commented Jul 26, 2020

@JJ Can you confirm that the tests themselves are correct?

@vrurg
Copy link
Contributor Author

vrurg commented Jul 26, 2020

Ok, considering the "thumb up" as the confirmation. :) Thank you!

@vrurg
Copy link
Contributor Author

vrurg commented Jul 27, 2020

Just as a note. I have updated the tool to do different backends. The following table is for both moar and jvm.

Key:
S..   = some tests passed
.P.   = plan ok (ran all tests)
..A   = all passed
    passed / planned or ran
+-----------------+-----------------+------------------------------------------------+
| jvm             | moar            |                                                |
+-----------------+-----------------+------------------------------------------------+
| ...     0/0     | ...     0/0     | S13-overloading/fallbacks-deep.t               |
| ...     0/0     | ...     0/0     | S13-overloading/multiple-signatures.t          |
| ...     0/0     | ...     0/0     | S16-unfiled/getpeername.t                      |
| .PA     0/21    | .PA     0/21    | S17-procasync/windows-arg-quoting.t            |
| SPA    28/30    | SPA    30/30    | S26-documentation/13-space-chars.t             |
| SPA     9/9     | SPA     9/9     | S26-documentation/12-non-breaking-space.t      |
| ...     0/0     | ...     0/0     | S01-perl-5-integration/modify_inside_p5.t      |
| ...     0/0     | ...     0/0     | S01-perl-5-integration/modify_inside_p5_p6.t   |
| SPA     3/3     | SPA     3/3     | S26-documentation/11-non-breaking-space.t      |
| .PA     0/4     | SPA     4/4     | S32-list/toggle.t                              |
| ...     0/0     | ...     0/0     | S12-traits/basic.t                             |
| ...     0/0     | ...     0/0     | S12-traits/parameterized.t                     |
| ...     0/0     | ...     0/0     | S03-operators/shortcuts.t                      |
| ...     0/0     | ...     0/0     | S12-class/open_closed.t                        |
| ...     0/0     | ...     0/0     | S13-syntax/aliasing.t                          |
| ...     0/0     | ...     0/0     | S24-testing/2-force_todo.t                     |
| ...     0/0     | ...     0/0     | S19-command-line-options/01-dash-uppercase-i.t |
| S..     2/2     | S..     2/2     | S24-testing/6-done_testing.t                   |
| SPA     9/9     | SPA     9/9     | S17-supply/Seq.t                               |
| S..     1/1     | S..     1/1     | S24-testing/1-basic.t                          |
| ...     0/0     | ...     0/0     | S10-packages/export.t                          |
| SP.     4/10    | SP.     4/10    | S32-io/child-secure.t                          |
| ...     0/2     | ...     0/2     | S10-packages/require-and-use--dead-file.t      |
| SPA     9/9     | SPA     9/9     | S10-packages/nested-use.t                      |
| ...     0/0     | ...     0/0     | t/fudgeandrun.t                                |
| SP.    22/23    | SP.    22/23    | S10-packages/scope.t                           |
| ...     0/0     | ...     0/0     | S05-modifier/ratchet.t                         |
| S..     2/51    | S..     2/51    | S05-modifier/exhaustive.t                      |
| S..     1/1     | SP.     3/4     | S17-supply/watch-path.t                        |
| ...     0/0     | ...     0/0     | S14-traits/package.t                           |
| ...     0/0     | ...     0/0     | S14-traits/variables.t                         |
| ...     0/0     | ...     0/0     | S05-capture/hash.t                             |
| ...     0/0     | ...     0/0     | S05-capture/external-aliasing.t                |
| ...     0/0     | SP.     6/8     | S11-modules/gh2979.t                           |
| ...     0/0     | ...     0/0     | S11-modules/use-perl-6.t                       |
| ...     0/0     | ...     0/0     | S05-nonstrings/basic.t                         |
| ...     0/0     | ...     0/0     | S11-modules/re-export.t                        |
| S..     1/2     | S..     1/2     | S12-attributes/augment-and-initialization.t    |
| ...     0/0     | ...     0/0     | S12-attributes/trusts.t                        |
| ...     0/0     | ...     0/0     | S14-roles/attributes-6e.t                      |
| SPA    10/10    | SP.     3/10    | t/test-util/01-is-eqv.t                        |
| SPA    41/41    | SPA    41/41    | S05-mass/properties-contributory.t             |
| ...     0/0     | ...     0/0     | S06-signature/multiple-signatures.t            |
| ...     0/0     | ...     0/0     | S06-signature/slurpy-blocks.t                  |
| ...     0/0     | ...     0/0     | S06-macros/returning-string.t                  |
| ...     0/0     | ...     0/0     | S06-macros/postfix.t                           |
| ...     0/0     | ...     0/0     | S04-phasers/interpolate.t                      |
| ...     0/0     | ...     0/0     | S06-macros/returning-closure.t                 |
| S..     1/1     | S..     1/1     | S04-phasers/exit-in-check.t                    |
| ...     0/0     | ...     0/0     | S04-statements/goto.t                          |
| ...     0/0     | ...     0/0     | S04-statements/leave.t                         |
| ...     0/0     | ...     0/0     | S04-statements/lazy.t                          |
| SPA     3/3     | SPA     3/3     | S14-roles/versioning.t                         |
| ...     0/0     | ...     0/0     | S32-temporal/time.t                            |
| ...     0/0     | ...     0/0     | S06-advanced/return_function.t                 |
| ...     0/0     | ...     0/0     | S06-advanced/dispatching.t                     |
| ...     0/0     | ...     0/0     | S06-advanced/caller.t                          |
| SPA   124/124   | SPA   124/124   | S03-sequence/exhaustive.t                      |
| SP.  1752/2204  | SP.  1752/2204  | S32-str/val.t                                  |
| SPA     9/9     | SPA     9/9     | S11-modules/versioning.t                       |
+-----------------+-----------------+------------------------------------------------+

Now I won't need to re-run it every time when terminal output is lost for whatever reason.

@vrurg
Copy link
Contributor Author

vrurg commented Jul 27, 2020

@tbrowder ping, please! I have a request to you.

@JJ
Copy link
Contributor

JJ commented Jul 27, 2020

@vrurg it actually meant I was going to do it. Doing it now. Most were added by @tbrowder , BTW.

  • 11-non-breaking-space.t. This was created as a test for this issue. I would do some minor refactoring of the test, but it tests what it says it does.
  • 12-non-breaking-space.t This is a more extensive version of the previous one, with more extensive testing of different non-breaking-spaces. It kind of supersedes the other, but I guess it's OK to have both.
  • 13-space-chars.t does not have anything to do with documentation, it mostly tests chr and ord. It's probably OK if you keep it off spec, and even if you do, it would make sense to move it to a different directory.

@vrurg
Copy link
Contributor Author

vrurg commented Jul 27, 2020

@JJ As you can see, I have already found the author. :)

I would consider merging 11 and 12 then. Besides, I wanted to ask @tbrowder if it's possible to use less EVALed code as with the current test code I send all my condolence to the one who would be debugging it!

But more direct problem with it is that if anything would ever require fudging then we're in trouble. I understand that the problem is about $=pod not being available outside of its compunit. But then something like this could be done:

sub do-test($pod) {
    isa-ok $pod, Pod::Block, "is a pod block";
}

EVAL q:to/TEST/;
    =begin pod
    =end pod
    do-test($=pod[0]);
    TEST

So, we only incapsulate our data inside the EVAL.

@vrurg
Copy link
Contributor Author

vrurg commented Jul 27, 2020

Update to the example from my last comment:

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

@tbrowder
Copy link
Member

tbrowder commented Jul 27, 2020

I will take the advice from you both and create a PR to put the corrected tests in spectest.data.

  • 13-space-chars.t does not have anything to do with documentation, it mostly tests chr and ord. It's probably OK if you keep it off spec, and even if you do, it would make sense to move it to a different directory.

As I recall, the test was added during discussions about pod and the non-breaking space problem with the pod renderers. The test investigates the wider range of most, if not all, of the Unicode space chars and lists the ones we use. It tests the round-trip handling and shows the normalizing effect on some of the chars.

I have no major problem with the test being moved but I certainly wouldn't dump it. I do think it should stay in the Spec.

@vrurg
Copy link
Contributor Author

vrurg commented Jul 27, 2020

@tbrowder That's exactly why am I asking you to rewrite it. Because whereas it can catch a bug now but it makes it extremely hard to diagnose and debug the bug using test code because of everything is EVALed. What I want to see is only the autogenerated pod to be under the EVAL and all the code working with it to be outside.

@vrurg
Copy link
Contributor Author

vrurg commented Jul 27, 2020

@tbrowder BTW, I would appreciate it if you can then produce a PR to #658.

@vrurg
Copy link
Contributor Author

vrurg commented Jul 27, 2020

@patrickbkr Is S17-procasync/windows-arg-quoting.t ready for spectest.data?

@patrickbkr
Copy link
Member

@vrurg Yes, those tests are good to go.

@tbrowder
Copy link
Member

tbrowder commented Jul 28, 2020

@tbrowder BTW, I would appreciate it if you can then produce a PR to #658.

See my PR #659.

Note I did not combine 11 and 12 but I can if you insist.

@vrurg
Copy link
Contributor Author

vrurg commented Aug 1, 2020

The table as it looks now:

+-----------------+-----------------+------------------------------------------------+
| jvm             | moar            |                                                |
+-----------------+-----------------+------------------------------------------------+
| SPA     3/3     | SPA     3/3     | S26-documentation/11-non-breaking-space.t      |
| ...     0/0     | ...     0/0     | S13-overloading/fallbacks-deep.t               |
| ...     0/0     | ...     0/0     | S01-perl-5-integration/modify_inside_p5.t      |
| ...     0/0     | ...     0/0     | S01-perl-5-integration/modify_inside_p5_p6.t   |
| .PA     0/21    | .PA     0/21    | S17-procasync/windows-arg-quoting.t            |
| ...     0/0     | ...     0/0     | S13-overloading/multiple-signatures.t          |
| ...     0/0     | ...     0/0     | S12-traits/basic.t                             |
| ...     0/0     | ...     0/0     | S12-traits/parameterized.t                     |
| ...     0/0     | ...     0/0     | S03-operators/shortcuts.t                      |
| ...     0/0     | ...     0/0     | S12-class/open_closed.t                        |
| S..     2/2     | S..     2/2     | S24-testing/6-done_testing.t                   |
| ...     0/0     | ...     0/0     | S19-command-line-options/01-dash-uppercase-i.t |
| ...     0/0     | ...     0/0     | t/fudgeandrun.t                                |
| S..     1/1     | S..     1/1     | S24-testing/1-basic.t                          |
| ...     0/2     | ...     0/2     | S10-packages/require-and-use--dead-file.t      |
| SPA     4/4     | SPA     4/4     | S14-traits/package.t                           |
| ...     0/0     | ...     0/0     | S05-capture/external-aliasing.t                |
| ...     0/0     | ...     0/0     | S05-capture/hash.t                             |
| ...     0/0     | ...     0/0     | S11-modules/use-perl-6.t                       |
| SPA     1/4     | SP.     3/4     | S17-supply/watch-path.t                        |
| SPA    10/10    | SP.     3/10    | t/test-util/01-is-eqv.t                        |
| ...     0/0     | ...     0/0     | S05-nonstrings/basic.t                         |
| ...     0/0     | ...     0/0     | S12-attributes/trusts.t                        |
| ...     0/0     | ...     0/0     | S06-signature/multiple-signatures.t            |
| ...     0/0     | ...     0/0     | S06-signature/slurpy-blocks.t                  |
| ...     0/0     | ...     0/0     | S11-modules/re-export.t                        |
| ...     0/0     | ...     0/0     | S06-macros/returning-string.t                  |
| S..     2/2     | S..     2/2     | S06-macros/returning-closure.t                 |
| S..     1/1     | S..     1/1     | S04-phasers/exit-in-check.t                    |
| ...     0/0     | ...     0/0     | S06-macros/postfix.t                           |
| ...     0/0     | ...     0/0     | S04-statements/goto.t                          |
| ...     0/0     | ...     0/0     | S32-temporal/time.t                            |
| ...     0/0     | ...     0/0     | S06-advanced/return_function.t                 |
| ...     0/0     | ...     0/0     | S06-advanced/caller.t                          |
| S..  1752/2179  | S..  1752/2179  | S32-str/val.t                                  |
+-----------------+-----------------+------------------------------------------------+

Of those:

  • S26-documentation/11-non-breaking-space.t – I wonder if it's superceded by S26-documentation/12-non-breaking-space.t
  • S14-traits/package.t and S17-procasync/windows-arg-quoting.t were overlooked and must be added into spectest.data

The rest requires filtering through problem-solving as I have doubts about the syntaxes or the semantics they're proposing.

@vrurg
Copy link
Contributor Author

vrurg commented Aug 1, 2020

@lizmat if you have time to look at S32-str/val.t I would very much appreciate it. Either our view at val() has changed over time and some tests should not be made; or the current implementation of val() is still broken.

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

No branches or pull requests

4 participants