Test class Generator

[lonnieezell]

Description

Adds a new make:test command to generate a skeleton test file. Required refactoring the GeneratorTrait to allow setting a namespace outside of the App directory to work. Also needed to add a Tests namespace by default to allow it be generated, but that's something we should have anyway for anyone writing their own tests. Surprised this hasn't been a pain point for others in the past, honestly.

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[lonnieezell]

Man, writing code for CI has become tedious. I'm having to fix a dozen PHPStan issues that have been around for ages in the GeneratorTrait, in code that I didn't touch for this PR. Really annoying. Can't imagine how annoyed I'd be if I was just trying to submit a small fix for the framework and wasn't on the core team.

And why is empty() not allowed anymore? Sometimes it's exactly what you need and mean. Grumble.

[michalsn]

I'm having to fix a dozen PHPStan issues that have been around for ages in the GeneratorTrait, in code that I didn't touch for this PR.

Yes, this is an issue for me too. But I'm not sure how we can handle it... Can lowering the level for phpstan can help here? This may be a real problem which holding people back from contributing.

If I remember correctly @paulbalandan advised me in the past to just generate the baseline

vendor/bin/phpstan analyse --generate-baseline phpstan-baseline.php

And why is empty() not allowed anymore?

I think we've established that in 99% of cases, it is too loose. And it brings a lot of trouble. I believe you can still add an exception for it.

[kenjis]

I didn't realize there could be so many errors, but these are technical debt in the trait.
It seems that the errors that had been suppressed erupted because that trait is used in many places.

I will fix them.

[kenjis]

@lonnieezell I don't know how you made commits in this PR.
There are only merge commits.

I was not able to extract your commit in this PR. I ran git rebase and all your work was gone.
So I cherry-picked and fixed the commit message. See #8334
If you want, checkout my branch, and edit commits as you like.

[kenjis]

The reason why empty() should not be used is that it's a very loose comparison.
Thereforeempty() causes bugs or security holes.

Also, developers who read the code later must consider what empty() really means.
It is a pain. However, when writing code, there should almost always be a narrower range of conditions, not empty() in the developer's mind.

Sometimes it's exactly what you need and mean.

See https://3v4l.org/BsbRk
If you use empty(), it means to pass these values.

[kenjis]

Also needed to add a Tests namespace by default to allow it be generated, but that's something we should have anyway for anyone writing their own tests.

It seems we don't need Teests in the following case:

$ ./spark make:test App\\Controllers\\HelloTest
...
File created: ROOTPATH/tests/App/Controllers/HelloTest.php

<?php

namespace Tests\App\Controllers;

use CodeIgniter\Test\CIUnitTestCase;

class HelloTest extends CIUnitTestCase
{
    protected function setUp(): void
    {
        parent::setUp();
    }

    public function testExample(): void
    {
        //
    }
}

[lonnieezell]

Sorry about the empty comment last night. It was mostly rhetorical and born out of my frustration. I understand what this team would not want it used. I don't find it nearly as dangerous but that's fine. And yes, after 17 years with PHP I am aware of the values it checks for.

@kenjis Thanks for fixing those. Wasn't looking for the help just expressing frustration. As someone who spent years of my spare time writing this imperfect code I don't find the process enjoyable anymore because something like this happens every time. We have to find a better balance between code quality checks and developer experience.

I'll look at the rest of the PR and comments tonight. Just getting my day started here.

[kenjis]

@lonnieezell No problem. It is good thing you can express your frustration.

The most easiest workaround is to update the baseline. This works every time.

vendor/bin/phpstan analyze --generate-baseline phpstan-baseline.php

But I really don't want empty() is used.

I thought all objects were not empty, but today I know an empty object.

php > $value = new SimpleXMLElement('<foo></foo>');
php > var_dump(empty($value));
php shell code:1:
bool(true)

[lonnieezell]

The most easiest workaround is to update the baseline. This works every time.

Right, but it works by hiding errors, partially defeating why we're using it and if we use it frequently don't we continually hide a bunch of errors?

But I really don't want empty() is used.

I have no problem with this, really, just find it a bit over-restrictive.

[kenjis]

Right, but it works by hiding errors, partially defeating why we're using it and if we use it frequently don't we continually hide a bunch of errors?

Yes, it is correct. So we should not use it frequently.

But we are hiding more than 1000 errors now.
And like this PR, there could show a lot of errors by a little change.
Well, it is rare to have so many errors, in that case, we may update the baseline.

Also, our PHPStan level is still 6.
It still takes time to reach the highest level. We can only improve little by little.
If we remove the baseline and set the level to max, there are 3405 errors in 4.5 branch now.
If there were zero errors now, there would have been even fewer errors occurring in this PR.

Of course, I do not recommend hiding errors.
However, it would be easier to develop with a policy of freely updating baselines to save frustration.
The differences in the baseline file will also show what errors have been suppressed.
So it is not such a bad method.

[lonnieezell]

All green.

[kenjis]

@lonnieezell I know it's a little late, but who is this command for, the developers who use appstarter?

If so, I think the current best practice is to match the namespace of the class file with the test file. However, this command cannot generate such a test file.

Also, this command cannot be used for CI4 development, because it cannot generate files under tests/system/.

[kenjis]

According to the docs, test classes are expected in tests/app.

In order to take advantage of the additional tools provided, your tests must extend CIUnitTestCase. All tests are expected to be located in the tests/app directory by default.

To test a new library, Foo, you would create a new file at tests/app/Libraries/FooTest.php:
https://codeigniter4.github.io/CodeIgniter4/testing/overview.html#the-test-class

[lonnieezell]

For app starter users. While I know that we do namespace based on class namespace I don't know that's a best practice. I haven't encountered that anywhere else and find it a little confusing since that's the only place that does not map the namespace to a directory within our codebase.

[lonnieezell]

According to the docs, test classes are expected in tests/app.

Then I think we should change the docs. We have no practical reason to enforce this and it just makes a slightly worse developer experience. Any time the user wants to run a single test class they have to type in extra characters for no beneficial reason.

My opinion is that the tests folder should be left to organize however they see fit.

[kenjis]

I know this is a bit different from your opinion, but what about #8374?