Skip to content

feat: improve CLI input testability #7978

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
Nov 5, 2023
Merged

feat: improve CLI input testability #7978

merged 17 commits into from
Nov 5, 2023

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Sep 25, 2023 •
edited
Loading

Needs #8137

Description
To improve testability.

  • add InputOutput and MockInputOutput class to CLI

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 docs needed Pull requests needing documentation write-ups and/or revisions. labels Sep 25, 2023
@kenjis kenjis changed the title feat: add InputOut class to CLI feat: improve CLI input testability Sep 26, 2023
@kenjis kenjis marked this pull request as draft September 26, 2023 03:31
@kenjis kenjis removed the docs needed Pull requests needing documentation write-ups and/or revisions. label Sep 26, 2023
@kenjis kenjis marked this pull request as ready for review September 26, 2023 05:16
@kenjis kenjis force-pushed the feat-cli-io branch 3 times, most recently from 050a396 to 392132f Compare September 26, 2023 05:30
@MGatner
Copy link
Member

MGatner commented Oct 10, 2023

Have to come back to this one.

@github-actions github-actions bot added the stale Pull requests with conflicts label Oct 12, 2023
@github-actions
Copy link

👋 Hi, @kenjis!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

@kenjis kenjis removed the stale Pull requests with conflicts label Oct 13, 2023
michalsn
michalsn approved these changes Oct 31, 2023
View reviewed changes
Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is indeed easier. I like the idea.

MGatner
MGatner requested changes Nov 4, 2023
View reviewed changes
@MGatner
Copy link
Member

MGatner commented Nov 4, 2023

I love! A few comments and suggestions.

@kenjis kenjis requested a review from MGatner November 5, 2023 00:13
@kenjis
Copy link
Member Author

kenjis commented Nov 5, 2023

Adding declare(strict_types=1) shows a hidden type error.

There was 1 error:

1) CodeIgniter\Commands\Database\ShowTableInfoTest::testDbTableShowsDBConfig
TypeError: CodeIgniter\CLI\InputOutput::input(): Return value must be of type string, bool returned

/home/runner/work/CodeIgniter4/CodeIgniter4/system/CLI/InputOutput.php:51
/home/runner/work/CodeIgniter4/CodeIgniter4/system/CLI/CLI.php:259
/home/runner/work/CodeIgniter4/CodeIgniter4/system/CLI/CLI.php:296
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Commands/Database/ShowTableInfo.php:134
/home/runner/work/CodeIgniter4/CodeIgniter4/system/CLI/Commands.php:65
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Common.php:198
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Commands/Database/ShowTableInfoTest.php:69

https://github.com/codeigniter4/CodeIgniter4/actions/runs/6757952382/job/18369383288?pr=7978

kenjis and others added 9 commits November 5, 2023 16:48
@kenjis
docs: add docs
53c76b6
@kenjis
test: update assertion
8014b6b
@kenjis @michalsn
docs: fix by proofreading
9ff8db2 
Co-authored-by: Michal Sniatala <michal@sniatala.pl>
@kenjis @MGatner
refactor: add final to MockInputOutput
086ca7f 
Co-authored-by: MGatner <mgatner@icloud.com>
@kenjis @MGatner
refactor: add strict_types
835462d 
Co-authored-by: MGatner <mgatner@icloud.com>
@kenjis @MGatner
docs: native @var tag
233f7e4 
Co-authored-by: MGatner <mgatner@icloud.com>
@kenjis @MGatner
docs: fix by proofreading
962777e 
Co-authored-by: MGatner <mgatner@icloud.com>
@kenjis
docs: add @testtag
2ef3169
@kenjis
refactor: add declare(strict_types=1) to new class
6767da7
@MGatner
Copy link
Member

MGatner commented Nov 5, 2023

Nice strict catch! I'm surprised PHPStan didn't complain. Thanks for the changes, this is looking great.

MGatner
MGatner approved these changes Nov 5, 2023
View reviewed changes
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meant to be "approve" - my comment is not required.

@kenjis kenjis merged commit cf6b9da into codeigniter4:4.5 Nov 5, 2023
@kenjis kenjis deleted the feat-cli-io branch November 5, 2023 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MGatner MGatner MGatner approved these changes

@michalsn michalsn michalsn approved these changes

Assignees
No one assigned
Labels
enhancement PRs that improve existing functionalities
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kenjis @MGatner @michalsn