Page MenuHomePhorge

Update PhutilCowsay.php to work for small cows
ClosedPublic

Authored by Sten on Sep 12 2023, 16:09.

Details

Summary

Update PhutilCowsay.php to work for small cows.

In doing so, we also simplify the code to just use multiline regexps rather than trying to parse a line at a time.

________________ < What about me? > ---------------- \ ,__, \ (oo)____ (__) )\ ||--|| *
Test Plan

Check cowsay works for the built in non-perl cow:

cowsay(cow='companion'){{{Built in}}}

Test all the perl cows:

cowsay(cow='bunny'){{{Testing bunny}}}
cowsay(cow='cower'){{{Testing cower}}}
cowsay(cow='daemon'){{{Testing daemon}}}
cowsay(cow='default'){{{Testing default}}}
cowsay(cow='dragon-and-cow'){{{Testing dragon-and-cow}}}
cowsay(cow='dragon'){{{Testing dragon}}}
cowsay(cow='elephant'){{{Testing elephant}}}
cowsay(cow='eyes'){{{Testing eyes}}}
cowsay(cow='flaming-sheep'){{{Testing flaming-sheep}}}
cowsay(cow='head-in'){{{Testing head-in}}}
cowsay(cow='kitty'){{{Testing kitty}}}
cowsay(cow='koala'){{{Testing koala}}}
cowsay(cow='meow'){{{Testing meow}}}
cowsay(cow='moofasa'){{{Testing moofasa}}}
cowsay(cow='moose'){{{Testing moose}}}
cowsay(cow='mutilated'){{{Testing mutilated}}}
cowsay(cow='satanic'){{{Testing satanic}}}
cowsay(cow='sheep'){{{Testing sheep}}}
cowsay(cow='skeleton'){{{Testing skeleton}}}
cowsay(cow='small'){{{Testing small}}}
cowsay(cow='squirrel'){{{Testing squirrel}}}
cowsay(cow='stegosaurus'){{{Testing stegosaurus}}}
cowsay(cow='supermilker'){{{Testing supermilker}}}
cowsay(cow='surgery'){{{Testing surgery}}}
cowsay(cow='turkey'){{{Testing turkey}}}
cowsay(cow='turtle'){{{Testing turtle}}}
cowsay(cow='tux'){{{Testing tux}}}
cowsay(cow='www'){{{Testing www}}}

Diff Detail

Repository
rARC Arcanist
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Sten requested review of this revision.Sep 12 2023, 16:09
Sten planned changes to this revision.Sep 12 2023, 16:10
src/utils/PhutilCowsay.php
44–59

Maybe just $matches (and comment optional)

src/utils/__tests__/cowsay/cube_perl.test
1–13

✅ I examined all the original perl-script cowfile(s) in /usr/share/cowsay/cows/* and I can confirm that all of them have that damn EOC (end of cow) so I'm OK with updating this test script, since the comment was about testing the "original per-script".

Replace $m with $matches, as per review.

Sten marked an inline comment as done.Oct 11 2023, 07:31

I'm trying to propose two unit tests, but I'm not able to make them "fail" before the change. The unit always says "OK" so I think I'm not useful :( But here as reference:

src/utils/__tests__/cowsay/small.expect
 ________________
< What about me? >
 ----------------
  \   ,__,
   \  (oo)____
      (__)    )\
         ||--|| *
src/utils/__tests__/cowsay/small.test
  $thoughts   ,__,
   $thoughts  ($eyes)____
      (__)    )\
         ||--|| *
~~~~~~~~~~
{
  "text": "What about me?"
}

I'm just using against:

arc unit src/utils/__tests__/PhutilCowsayTestCase.php

Before and after this change.

Any idea why is that unit test always resulting "OK" to me?

Anyway, feel free to adapt and pull this inside your change 👍

smallcow test case added.

Test fails under the previous code with:

$ arc unit
   FAIL  PhutilCowsayTestCase::testCowsay
Assertion failed, expected values to be equal (at PhutilCowsayTestCase.php:57): Cowsay Test Case "small.test"
Expected vs Actual Output Diff
--- Old Value
+++ New Value
@@ -1,7 +1,8 @@
 ' __________________
 < How are my eyes? >
  ------------------
+oo = ".." unless (oo);
        \   ,__,
         \  (oo)____
            (__)    )\
               ||--|| *'

Turns out we can simply copy the perl template file, then add in the seperator and test values.

Sten marked an inline comment as done.Oct 12 2023, 16:13

Any problems with this fix?

So, no PHP changes? Interesting.

Thank you so much :) Tested intensively.

This revision is now accepted and ready to land.Nov 13 2023, 07:46

I'm a bit confused (!) I thought there were no PHP changes on Rev 1414 and I approved it, but instead I now see that we have again PHP changes. It's unclear to me whenever the Diff I approved was "updated to reflect changes", or maybe I'm just totally crazy because of real life. Please share your opinion.

In any case, thanks, and:

whatcouldgowrong