Replace for loops on JSON test cases with pure Perl test cases
janlimpens opened this issue · 11 comments
Hi,
some months ago, when I started my job as a Perl programmer, in an effort to polish my skills in this programming language and having used other language tracks to do the same, I tried entering the Perl track. Unfortunately, this has been a very frustrating experience. This was due to the very odd testing style employed by the track, using a hash in DATA and evaluate it using some opaque logic.
Maybe, this works well for the very first exercises, where you do not have to look at the implementation of the tests to complete. However, in later exercises it is not detailed, which function expects which input and what it should return, you just get whatever $input and return whatever $output. This is, when you start looking at tests and with it, the headaches.
In all other languages, I have done exercises, these have explained this to me, but look at the code for
https://exercism.org/tracks/perl5/exercises/allergies:
for my $case (@test_cases) {
if ( $case->{property} eq 'allergicTo' ) { # what property is it talking about
is allergic_to( $case->{input} ), $case->{expected} ? T : DF, #T?? DF?? DAFUQ?
$case->{description}; # huh??
}
elsif ( $case->{property} eq 'list' ) {
is
list_allergies( $case->{input}{score} ), bag { # whoa bag???
item $_ for @{ $case->{expected} };
end;
}, $case->{description};
}
}
__DATA__
[
{
"description": "testing for eggs allergy: not allergic to anything",
"expected": false,
"input": {
"item": "eggs",
"score": 0
},
"property": "allergicTo"
}, ...I have come across this testing style mainly on code older than 15 years and even now, having written thousands of lines in Perl, this gives me a headache just by looking at it. How is a beginner in Perl, who this exercise is aiming for, supposed to understand what is going on?
How about a much simpler and intelligible
subtest 'testing for eggs allergy' => sub() {
ok (!allergic_to('eggs', 0), 'not allergic to anything');
# ^ wants bool ^ has this signature
ok(allergic_to('eggs', 1), 'allergic only to eggs'); # assuming egg allergy is 1
ok(allergic_to('eggs', 3), 'allergic to eggs and the other thing');
ok(!allergic_to('eggs', 2), 'allergic to the other thing');
};On second look, the actual signature is more convoluted, still, taking a hash out of some reason. Perlcritic must be sobbing desperately, here.
The exercise should not be about trying to decipher the test expectations hash, but about the problem at hand. This here, I am sorry to say, only serves to worsen Perl's bad reputation of being a cryptic and antiquated language, and unnecessarily so!
This said, I'd be willing to help.
I agree with this issue. While the DATA hash makes it easier to update the test cases, it really puts a burden on the student.
I went through the exact same thing on the Tcl track, deciding to make the tests simpler and self-contained. Here's the evolution of the Tcl allergies exercises: exercism/tcl@44c8ea4
I also will be willing to pitch in.
The quality of the tests are certainly an issue. There are quite a few exercises which, outside of a few functional updates, have seen very little change since Exercism V1.
I'm split between maintaining both the Perl and Raku tracks and unfortunately do not have as much free time as I'd like.
IIRC, exercises were ordered in a certain way on earlier versions of Exercism. In earlier exercises (leap I believe?) it was noted that T and DF were meant for True and Defined but False, and hello-world also noted what each argument to is was for. It would be nice to have more documentation in the track for this.
Using is rather than ok does have some benefits, primarily a much more detailed test output, such as telling you what your subroutine returned (which was recently added to the test runner on the site). ok on the otherhand gives you nothing but a pass or fail, plus the test description, plus doesn't consider definedness unless you're explicit about it:
1..2
not ok 1 - &ok test
# Failed test '&ok test'
# at test.t line 4.
not ok 2 - &is test
# Failed test '&is test'
# at test.t line 5.
# +-----+--------+-------+-----+
# | GOT | OP | CHECK | LNs |
# +-----+--------+-------+-----+
# | 0 | TRUE() | TRUE | 5 |
# +-----+--------+-------+-----+
Using $input as a parameter I agree is best avoided. At the time I suspect I may have been updating multiple exercises at once and used $input to rapidly get through as many updates as I could.
I'm open to making improvements, but I'm currently still in the process of making sure the test runner on V3 is working properly on each exercise. Subtests can be problematic for the test runner at the moment so I'm trying to avoid their use where I can.
I think there can be a solution where tests like your suggestion can be generated dynamically. If that can be achieved then I'm for it, else without another regular maintainer I think it would be too much work to keep up with should tests change or the site has future updates.
It wouldn't yet work for an exercise like allergies, since the tests are not exclusively boolean (although I have an idea on how to approach that), but I have a draft for creating something to automatically write out boolean tests:
An idea struck me and I've added a feature to the exercise generator, allowing for writing an eval to create tests for specified properties.
An example for allergies:
https://github.com/exercism/perl5/blob/601087d4059db03c93d7800f54eb4bd77ba53695/exercises/practice/allergies/allergies.t
In PR #468
(perltidy is currently responsible for code alignment, am open to suggestions for configuration on that).
I've merged the PR above which introduces the changes to the leap, nucleotide-count, and allergies exercises. Other exercise will also be updated going forward but this may take a while!
This is a work in progress. I've been going through each exercise alphabetically and updating the test cases accordingly. food-chain is the latest exercise I've created a PR for at the current time: #490
Can issues be marked as a draft? I'm still intending to get through the remainder of the exercises, but have a lot less free time currently!
I don't think we can mark it as "draft" for issues on the platform, but we can change the title to [WIP] to indicate it manually.
This issue should now be resolved pending the merge of remaining open PRs.