symfony1
symfony1 copied to clipboard
FriendsOfSymfony1
fix(test): exit code of lime test
Lime test should exit with non-zero code on failure.
Before this patch, it is not the case.
This patch is also included in #302, isn't it?
Yes, but, I suggest merging this one before #302.
@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.
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
Return types for private methods! Nice! That's what I was thinking, that I could do it everywhere in the framework. Thank you!
Apart from the fact that it's red now, I'm happy with the state of it! Well done!
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.
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.
@thePanz it's done
See #363 for topic patches of test environment that allow me to work on this.
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".
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".
@thePanz ask to fix code structure, that's what I have done, not overall but only on changes location.
Too much, refactoring was done?
The latest step was extracting lime_test and lime_harness on dedicated files, then fix CS on it.
Are tests understandable?
The diff before class extraction https://github.com/FriendsOfSymfony1/symfony1/pull/301/files/fbc40e7396a0f0da445585c57cc2486d243d75ae
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?
@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
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
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.
@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?
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.
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. #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?
@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.
Okay, @connorhu thanks for your feedback.
Let's go, I will keep open this PR with force push when step 1 is done.