exercism / php

Exercism exercises in PHP.
https://exercism.org/tracks/php
MIT License
144 stars 139 forks source link

Test passes locally but fails online #855

Closed dexmexter closed 18 hours ago

dexmexter commented 3 days ago

This particular test fails online but passes locally on my machine. My solution was very similar to one of the community solutions (https://exercism.org/tracks/php/exercises/clock/solutions/zembrowski) so I tried submitting an iteration that is identical and this test still shows failure for me.

Please note that this is the first time I've ever submitted a GitHub issue so I'm uncertain what type of formatting I should use or what further information would be valuable to include.

https://github.com/exercism/php/blob/92ef74bb9339fa639d3ffe40ed7c1c20a70d5209/exercises/practice/clock/ClockTest.php#L120-L127

mk-mxp commented 2 days ago

Hi @dexmexter! Thanks for reporting the issue. It would be helpful to have the failure message you see. Copy&paste it in a code block (the <> icon on the editor or Markdown ``` before / after it on their own lines).

From a short look at the solution, it might miss : string in the required method signature:

public function __toString(): string

This is a new requirement of PHP in the current versions, and the solution dates before that.

dexmexter commented 2 days ago

Hello @mk-mxp Here is the output I'm seeing from the website test results:

Code Run

$clock = new Clock(6, 15);

$clock = $clock->add(-160);

$this->assertEquals('03:35', $clock->__toString());

Test Failure

ClockTest::testSubtractMoreThanTwoHoursWithNegativeAdd
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'03:35'
+'08:55'

ClockTest.php:126

What is really odd is that it reports failure because it says that the solution returns '08:55' for that test.

tomasnorre commented 2 days ago

For me it also fails locally, with the copy/pasted code from https://exercism.org/tracks/php/exercises/clock/solutions/zembrowski

Running tests for: clock
PHPUnit 10.5.33 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.12

..........F..................                                     29 / 29 (100%)

Time: 00:00.017, Memory: 8.00 MB

There was 1 failure:

1) ClockTest::testSubtractMoreThanTwoHoursWithNegativeAdd
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'03:35'
+'08:55'

And i think it's correct that it's failing. To me it looks like the check for negative minutes are missing. At least compared to my solution https://exercism.org/tracks/php/exercises/clock/solutions/tomasnorre

public function add(int $minutes): Clock
{
    if (0 > $minutes) {
        return $this->sub(abs($minutes));
    }
    $this->time->modify("+{$minutes} minutes");
    return $this;
}

I'm on PHP 8.3 locally.

dexmexter commented 2 days ago

@tomasnorre I see now that "+{$minutes} minutes" would evaluate to "+-160 minutes" and I agree that it would be better to handle this as you've suggested.

I don't understand why this would pass locally (php version is 8.3.13, phpunit version 10.5.37) but not online.

tomasnorre commented 1 day ago

I don't understand why this would pass locally (php version is 8.3.13, phpunit version 10.5.37) but not online.

I haven't looked into that. I have this repository (https://github.com/exercism/php) checkout locally and replaced the linked solution with the on in https://github.com/exercism/php/blob/main/exercises/practice/clock/.meta/example.php then ran the tests with composer test:run.

Not 100% sure what happens in the online test runner. I haven't really touched that part of the code base yet.

tomasnorre commented 1 day ago

@tomasnorre I see now that "+{$minutes} minutes" would evaluate to "+-160 minutes" and I agree that it would be better to handle this as you've suggested.

I haven't looked that much into the linked solution, that i can say it mine is better or not, just thought that part was missing. I'm btw. on PHP 8.3.12 and PHPUnit 10.5.33 so both older than yours.

mk-mxp commented 1 day ago

OK, this is caused by a change in PHP 8.2.0. You can find it in the PHP docs on DateTime formats at the very end: number no longer accepts multiple signs, e.g. +-2. There is no specification of the current behaviour for this, but obviously it differs between PHP flavours (online test runner uses 8.3.10-cli-alpine3.20).

zembrowski's solution passed the online test runner 4 years ago, for sure using PHP < 8.2 which wasn't available at that time.

dexmexter commented 18 hours ago

I'll close the issue since it is actually a problem with my solution and not the testing suite. I feel a bit silly knowing that, but am very appreciative of the time you both took to help me discover the root cause.