Skip to content

feat: spark make:test creates test files in /tests/ directory #8374

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 7 commits into from

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Dec 28, 2023

Description

  • add namespace paths for test classes
    • add tests/app for namespace App
    • add tests/system for namespace CodeIgniter
  • make:test creates test files in /tests/ directory

E.g.,

$ ./spark make:test App\\Controllers\\FooTest 
File created: ROOTPATH/tests/app/Controllers/FooTest.php
$ ./spark make:test Controllers\\BarTest
File created: ROOTPATH/tests/app/Controllers/BarTest.php
$ ./spark make:test Commands\\FooTest --namespace CodeIgniter
File created: ROOTPATH/tests/system/Commands/FooTest.php

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

@kenjis kenjis added enhancement PRs that improve existing functionalities 4.5 labels Dec 28, 2023
@kenjis kenjis mentioned this pull request Dec 28, 2023
5 tasks
@lonnieezell
Copy link
Member

I said it elsewhere but I'll repeat it here for context.

I'm not a fan of forcing tests into the app directory within tests. It doesn't serve any practical purpose for the developer since they won't have a system directory within tests that it needs to be separate from. It does serve to slightly degrade developer experience since they'll have extra to type when running a single test file in isolation. I don't feel it's needed to force the developer's hand in how they organize their tests. I understand the idea is to allow mirroring of app/* classes into the tests folder but it does so by adding an additional folder to the tests, when it's very simple to mirror the layout when app and tests folder are both considered the top level directory and all directories are created within them.

As this PR is written the following makes no sense to me:

$ ./spark make:test FooTest 
File created: ROOTPATH/tests/app/FooTest.php
$ ./spark make:test Bar/FooTest 
File created: ROOTPATH/tests/app/Bar/FooTest.php

As written I cannot agree to this PR.

I could accept a compromise where the PR works as written ONLY when the filepath provided contains a namespace (identified by the presence of a backslash) or has the namespace forcibly defined. But when used without a namespace then it should leave the app directory out of the created path.

@michalsn
Copy link
Member

I agree with lonnieezell - forcing the tests/app folder seems unnecessary. That would make some sense only for people who download this repo and start their project like that. This is very uncommon (hopefully) as we promote the appstarter.

@kenjis
Copy link
Member Author

kenjis commented Dec 28, 2023

Okay, then for example, what is the test classname for App\Controllers\Home?
Tests\Controllers\HomeTest?

@michalsn
Copy link
Member

IMO if the project is small it could be just Tests\HomeTest and if bigger the use of Tests\Controllers\HomeTest is more reasonable. But at the end of the day, it's up to the developer.

@kenjis
Copy link
Member Author

kenjis commented Dec 28, 2023

@michalsn In that case, the current implementation (4.5) is fine because it forces the test class namespace to Tests.
However, if possible, it would be better to be able to handle various patterns by specifying options.
E.g., App\Controllers\HomeTest is no problem to me.

Also, now we provide sample test files in appstarter.
What should we do with these?
https://github.com/codeigniter4/CodeIgniter4/tree/develop/admin/starter/tests

@michalsn
Copy link
Member

@kenjis I have nothing against adding options to customize things.

The current tests are fine to me. They are supposed to showcase how we can test different areas of our application and they do the job, they're nicely grouped and easy to understand.

@kenjis
Copy link
Member Author

kenjis commented Dec 30, 2023

Go to #8388

@kenjis kenjis closed this Dec 30, 2023
@kenjis kenjis deleted the feat-make-test-namespace branch December 30, 2023 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants