fix(test): exit code of lime test #301

alquerci · 2024-01-01T09:37:33Z

Lime test should exit with non-zero code on failure.

Before this patch, it is not the case.

alquerci · 2024-01-19T18:37:05Z

Rebased on master branch

thirsch

This patch is also included in #302, isn't it?

alquerci · 2024-03-04T19:39:41Z

This patch is also included in #302, isn't it?

Yes, but, I suggest merging this one before #302.

thePanz · 2024-03-22T12:08:21Z

lib/vendor/lime/lime.php

@@ -155,6 +161,31 @@ public function __destruct()
    }

    flush();
+
+    self::$allExitCode |= $this->getExitCode();


this is a binary operation, done on an integer value, we should cast it to a bool before, or rename getExitCode() to isTestFailed()

WDYT?

thePanz · 2024-03-22T12:10:57Z

lib/vendor/lime/lime.php

@@ -24,9 +24,13 @@ class lime_test
  protected $options = array();

  static protected $all_results = array();
+  static private $instanceCount = 0;
+  static private $allExitCode = 0;


what about $overallSuccessful = true?

thePanz · 2024-03-22T12:12:50Z

lib/vendor/lime/lime.php

+    $plan = $this->results['stats']['plan'];
+    $failed = count($this->results['stats']['failed']);
+    $total = $this->results['stats']['total'];
+    is_null($plan) and $plan = $total and $this->output->echoln(sprintf("1..%d", $plan));


can we rewrite this as an if statement?

thePanz · 2024-03-22T12:13:51Z

lib/vendor/lime/lime.php

+    if ($failed)
+    {
+      return 1;
+    }
+    else if ($total == $plan)
+    {
+      return 0;
+    }
+
+    return 1;


Suggested change

if ($failed)

{

return 1;

}

else if ($total == $plan)

{

return 0;

}

return 1;

if (0 !== $failed) {

return 1;

}

return $total == $plan ? 0 : 1;

alquerci · 2024-03-25T19:50:48Z

@thePanz thanks for your review.

Mainly, your feedbacks are only about having better code structure.

So I will proceed with fixing code structure.
Then I will ping you when done.

alquerci · 2024-03-25T20:05:25Z

After rebase on master branch, I got this error.

$ test/bin/test 
+ docker-compose build
ERROR: The Compose file is invalid because:
Service php81 has neither an image nor a build context specified. At least one must be provided.

I know how to fix it.
But I am curious, if there are new prerequisites for testing.

https://github.com/FriendsOfSymfony1/symfony1/blob/master/README.md?plain=1#L61-L64

connorhu · 2024-03-22T11:20:59Z

lib/vendor/lime/lime.php

+    self::$allExitCode |= $this->getExitCode();
+
+    if (0 === self::$instanceCount) {
+        exit(self::$allExitCode);


Suggested change

exit(self::$allExitCode);

exit(self::$allExitCode);

connorhu · 2024-03-22T11:30:36Z

lib/vendor/lime/lime.php

+    $plan = $this->results['stats']['plan'];
+    $failed = count($this->results['stats']['failed']);
+    $total = $this->results['stats']['total'];
+    is_null($plan) and $plan = $total and $this->output->echoln(sprintf("1..%d", $plan));


Can you please improve the readability of this. This line makes me think of perl or bash code before php.

Or is it a valuation that is a mistake? 🤔

connorhu · 2024-03-26T05:59:19Z

lib/vendor/lime/lime.php

+  const STATE_PASS = 0;
+  const STATE_FAIL = 1;
+


Please add visibility.

connorhu · 2024-03-26T06:00:49Z

test/bin/test

-#
-dependencyPreferences='highest'
-skipPHPVersions='php8'
+main ()


Please try to keep the context of the PR.

connorhu · 2024-03-26T06:01:05Z

docker-compose.yml

@@ -50,6 +50,7 @@ services:
  php81:
    <<: *services_php74
    build:
+      context: .docker


Please try to keep the context of the PR.

connorhu · 2024-03-27T05:14:18Z

Return types for private methods! Nice! That's what I was thinking, that I could do it everywhere in the framework. Thank you!

connorhu · 2024-03-27T06:45:28Z

lib/vendor/lime/lime.php

@@ -952,8 +1015,7 @@ function lime_shutdown()
      );

      ob_start();
-      // see http://trac.symfony-project.org/ticket/5437 for the explanation on the weird "cd" thing


I'm copying the content of the trac ticket here to keep what it was:

[PATCH] lime.php not working on windows when there are spaces in php interpreter path

lime.php, line 541

passthru(sprintf('%s "%s" 2>&1', $this->php_cli, $file), $return);

the command fails if the path to php interpreter contains spaces, this obviously prevent all test to run.

In order to work with executables in directories with spaces it should be

passthru(sprintf('"%s" "%s" 2>&1', $this->php_cli, $file), $return);

but this fails too on windows: system/exec/passthru all fail when more than two arguments are quoted (it seems a php bug but I couldn't find any reference apart from some comments on the system function page)

In order to work on windows it should be written as

passthru(sprintf('""%s" "%s"" 2>&1', $this->php_cli, $file), $return);

this works on windows but still fails on linux (note the double "double quotes": I need to quote the entire command not just the executable and the argument)

The only workaround I could find was something I found on the comments of the system function that is adding a "nop" command call before the quoted command

passthru(sprintf('cd & "%s" "%s" 2>&1', $this->php_cli, $file), $return);

connorhu · 2024-03-27T20:49:26Z

Apart from the fact that it's red now, I'm happy with the state of it! Well done!

alquerci · 2024-03-28T19:20:02Z

It was not only about code structure, but the behavior has been exposed with the proof that work as expected (tests).

The current tests that failed introduce the fact that it should treat PHP errors/warnings/... as test failure with failed status code.

This failed test is like a marker to tell me what's next, for the next coding session.

Now I wonder if that is it useful to expose the behavior when an Exception or an Error is thrown.

As the migration to PHPUnit will make this obsolete.
Anyway, that's a good training for improving legacy code structure, like a kata.

connorhu · 2024-03-29T06:02:44Z

Now I wonder if that is it useful to expose the behavior when an Exception or an Error is thrown.

If hidden errors occur, yes. If valuable information about the state of the code is obtained, yes. If it just generates noise, no. The effects of such a change are only visible to you at the moment.

As the migration to PHPUnit will make this obsolete.

What you did was not useless, because it will take a long time to migrate all the tests under phpunit. Looking at the current tempo, an optimistic estimate is that one tenth of the tests will be moved to phpunit this year.

alquerci · 2024-03-31T21:26:01Z

The diff before class extraction https://github.com/FriendsOfSymfony1/symfony1/pull/301/files/fbc40e7396a0f0da445585c57cc2486d243d75ae

thePanz · 2024-04-03T08:15:09Z

There was a point when PR was doing perfectly what you set out to do. But the changes that have been made since then have made it impossible to follow what PR is actually doing. In my language, there is a saying that "fall off the other side of the horse".

@alquerci I agree here: the class extraction is a good thing to have, but makes this PR quite hard to follow. yes, maybe this is a "too much refactoring". WDYT?

alquerci · 2024-04-03T09:01:02Z

@thePanz I agree too.

What do you suggest?
Does it needs to extract the class extraction on another PR, or it is enough to have it on a dedicated commit like now?

Related to #356

connorhu · 2024-04-04T10:17:46Z

I propose a separate PR for cleaning, cs-fix and separation.

alquerci · 2024-04-04T19:09:44Z

I propose a separate PR for cleaning, cs-fix and separation.

To rephrase

One PR: make it work
- with clean lime tests
- with minimum code to make tests passes
- with all commit history kept that show each baby steps
One PR: make it clean
- with extract methods
- with move classes from big file to dedicated files
  - New files comes with CS fix
- with all commit history kept that show each baby steps

That's a nice kata, I like it.

@connorhu Should I do it?

How I will do it?

First PR

Delete all new production code, but keep the tests
Commit
Change tests expectations to make them passes
Change expectations one by one to reach new behaviour
One by one make them all passes
All of that steps without any cleaning

The code will be hard to read, but tests will pass.

Second PR

Create a PR based on first PR
This will prevent any modification to first PR, to avoid extra work when rebase.
Clean the code around diff of the first PR

OR

Wait for the first PR to be merged
Start cleaning

connorhu · 2024-04-05T06:08:47Z

If clean code is a prerequisite for successful test execution, the PR order is: cleanup, exit code. If clean code is not a prerequisite for successful test execution: exit code, cleanup.
Am I missing something? Maybe I'm missing something.

alquerci · 2024-04-05T07:47:04Z

@connorhu It is something between.

To be able to clean the code (having a better code structure).
=> The prerequisite is to have a test suite that you trust, if it pass you can ship it.

After that a trusted test suite is in place.
=> You can clean it, but it will be useless to touch code that does not need to be changed for fixing the exit code.

After that you make one small test pass
And if you do not clean the test then the production code.
=> It will be even harder to clean them all at the end.
=> It will be harder to make all tests pass in short term, with code hard to modify.

Instead, start to make one small step to the new behaviour (as steps I used on this PR).

Change one expectation
Make it pass with the minimal code change
Make test code clean
Make production code clean
Change second expectation
Make it pass with the minimal code change
Make test code clean
Make production code clean
N. ...

So here, I see no problem to restart the work on this.
This time I will keep all commit history.
But without cleaning the code progressively it will be a challenge that I accept.

What do you think?

connorhu · 2024-04-05T08:55:56Z

You can clean it, but it will be useless to touch code that does not need to be changed for fixing the exit code.

Here's what we think differently about. There are many PRs that are created because we see something in the code as a cleaning, tidying up process. The fact that lime classes are placed in a file is incorrect in the current practice of symfony core. The fact that indentation is 2 spaces is actually incorrect according to the current code base of symfony core.

So we need one (or two) organizer PRs that will eliminate this inconsistency. There is no need for these PRs to be created because of the "exit code" issue. If I previously notice lime using 2 space indentation I will have already cleaned it up (or added the rule to editorconfig).

If you want to fix the "exit code" problem, you must do it in a way that the fix is 100% understandable and transparent (so you must do the cleanup first or last).

Regardless of whether we're talking about 2-3 PRs, the end result of PRs in a row is that you have a more reliable test system, so you've achieved your goal.

To me, a measure of the reliability of a change is a) the code logic is unchanged (processes that are done by programs of proven quality, e.g. rector, cs-fixer) b) the change is verified by tests.

alquerci · 2024-04-05T18:25:10Z

I am closing this PR.

In order to provide a fix 100% understandable and transparent.

I will create a new PR with the proof of the current behaviour.
a. fix(test): exit code of lime test -- add proof of current behaviour #371
Then another PR, with the change on the behaviour.
Then another PR, with the local cleaning.
Then let an organizer to make a PR, with the structural cleaning.

@connorhu @thePanz does it's steps are ok for you?

connorhu · 2024-04-09T07:38:07Z

@alquerci I don't think you should close this PR. If I'm right, this is step 2 of 4. The 4 steps sketched out seem logical to me.

alquerci · 2024-04-09T10:21:52Z

Okay, @connorhu thanks for your feedback.

Let's go, I will keep open this PR with force push when step 1 is done.

alquerci · 2024-04-09T22:57:37Z

1st step done

#371

alquerci force-pushed the fixtest-exit-code-of-lime-test branch from f4f102e to 46a8d60 Compare January 1, 2024 09:41

alquerci changed the title ~~[WIP] fix(test): exit code of lime test~~ fix(test): exit code of lime test Jan 1, 2024

alquerci mentioned this pull request Jan 1, 2024

fix: PHP warnings triggered by the test suite #302

Open

alquerci force-pushed the fixtest-exit-code-of-lime-test branch from 46a8d60 to a60ff02 Compare January 5, 2024 20:38

thePanz force-pushed the fixtest-exit-code-of-lime-test branch from a60ff02 to 05ac333 Compare January 17, 2024 23:42

alquerci force-pushed the fixtest-exit-code-of-lime-test branch from 05ac333 to 154bc18 Compare January 19, 2024 18:36

thirsch approved these changes Feb 28, 2024

View reviewed changes

alquerci force-pushed the fixtest-exit-code-of-lime-test branch from 154bc18 to 136b3c1 Compare March 4, 2024 19:39

thirsch approved these changes Mar 4, 2024

View reviewed changes

connorhu approved these changes Mar 22, 2024

View reviewed changes

thePanz reviewed Mar 22, 2024

View reviewed changes

thePanz requested changes Mar 22, 2024

View reviewed changes

thePanz reviewed Mar 22, 2024

View reviewed changes

alquerci force-pushed the fixtest-exit-code-of-lime-test branch from 136b3c1 to dd6a51c Compare March 25, 2024 20:06

connorhu suggested changes Mar 26, 2024

View reviewed changes

alquerci force-pushed the fixtest-exit-code-of-lime-test branch 2 times, most recently from fa11b69 to f25c0cd Compare March 27, 2024 00:45

alquerci marked this pull request as draft March 27, 2024 00:46

connorhu reviewed Mar 27, 2024

View reviewed changes

alquerci force-pushed the fixtest-exit-code-of-lime-test branch from f25c0cd to d094b34 Compare March 27, 2024 20:15

alquerci force-pushed the fixtest-exit-code-of-lime-test branch from d094b34 to c8ff5bc Compare March 29, 2024 22:53

alquerci added 15 commits March 31, 2024 23:13

fix(test): exit code of lime test

f1d8713

fixup! fix(test): exit code of lime test

4c1f5a6

fixup! fix(test): exit code of lime test

cdd26d3

fixup! fix(test): exit code of lime test

9a64e90

fixup! fix(test): exit code of lime test

a22757e

fixup! fix(test): exit code of lime test

0cb14a9

fixup! fix(test): exit code of lime test

8312848

fixup! fix(test): exit code of lime test

3b709a4

fixup! fix(test): exit code of lime test

cc441d9

fixup! fix(test): exit code of lime test

824471f

fixup! fix(test): exit code of lime test

030d2e7

fixup! fix(test): exit code of lime test

fbc40e7

fixup! fix(test): exit code of lime test

cfc232f

fixup! fix(test): exit code of lime test

a815972

fixup! fix(test): exit code of lime test

d943f48

alquerci force-pushed the fixtest-exit-code-of-lime-test branch from 7e19aea to d943f48 Compare March 31, 2024 21:21

alquerci mentioned this pull request Apr 9, 2024

[i18n] sfMessageSource_MySQL is broken #347

Open

alquerci mentioned this pull request Apr 9, 2024

fix(test): exit code of lime test -- add proof of current behaviour #371

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(test): exit code of lime test #301

fix(test): exit code of lime test #301

alquerci commented Jan 1, 2024 •

edited

Loading

alquerci commented Jan 19, 2024

thirsch left a comment

alquerci commented Mar 4, 2024

thePanz Mar 22, 2024

thePanz Mar 22, 2024

thePanz Mar 22, 2024

thePanz Mar 22, 2024 •

edited

Loading

alquerci commented Mar 25, 2024

alquerci commented Mar 25, 2024

connorhu Mar 22, 2024

connorhu Mar 22, 2024

connorhu Mar 22, 2024

connorhu Mar 26, 2024

connorhu Mar 26, 2024

connorhu Mar 26, 2024

connorhu commented Mar 27, 2024

connorhu Mar 27, 2024 •

edited

Loading

connorhu commented Mar 27, 2024

alquerci commented Mar 28, 2024

connorhu commented Mar 29, 2024

alquerci commented Mar 31, 2024

thePanz commented Apr 3, 2024

alquerci commented Apr 3, 2024 •

edited

Loading

connorhu commented Apr 4, 2024 •

edited

Loading

alquerci commented Apr 4, 2024 •

edited

Loading

connorhu commented Apr 5, 2024

alquerci commented Apr 5, 2024

connorhu commented Apr 5, 2024

alquerci commented Apr 5, 2024 •

edited

Loading

connorhu commented Apr 9, 2024

alquerci commented Apr 9, 2024

alquerci commented Apr 9, 2024

fix(test): exit code of lime test #301

Are you sure you want to change the base?

fix(test): exit code of lime test #301

Conversation

alquerci commented Jan 1, 2024 • edited Loading

alquerci commented Jan 19, 2024

thirsch left a comment

Choose a reason for hiding this comment

alquerci commented Mar 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thePanz Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

alquerci commented Mar 25, 2024

alquerci commented Mar 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorhu commented Mar 27, 2024

connorhu Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

connorhu commented Mar 27, 2024

alquerci commented Mar 28, 2024

connorhu commented Mar 29, 2024

alquerci commented Mar 31, 2024

thePanz commented Apr 3, 2024

alquerci commented Apr 3, 2024 • edited Loading

connorhu commented Apr 4, 2024 • edited Loading

alquerci commented Apr 4, 2024 • edited Loading

To rephrase

How I will do it?

First PR

Second PR

connorhu commented Apr 5, 2024

alquerci commented Apr 5, 2024

connorhu commented Apr 5, 2024

alquerci commented Apr 5, 2024 • edited Loading

connorhu commented Apr 9, 2024

alquerci commented Apr 9, 2024

alquerci commented Apr 9, 2024

alquerci commented Jan 1, 2024 •

edited

Loading

thePanz Mar 22, 2024 •

edited

Loading

connorhu Mar 27, 2024 •

edited

Loading

alquerci commented Apr 3, 2024 •

edited

Loading

connorhu commented Apr 4, 2024 •

edited

Loading

alquerci commented Apr 4, 2024 •

edited

Loading

alquerci commented Apr 5, 2024 •

edited

Loading