Skip to content
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

[Help Wanted] Increasing test coverage of Command classes #10796

Open
Seldaek opened this issue May 24, 2022 · 25 comments · Fixed by #11547
Open

[Help Wanted] Increasing test coverage of Command classes #10796

Seldaek opened this issue May 24, 2022 · 25 comments · Fixed by #11547

Comments

@Seldaek
Copy link
Member

Seldaek commented May 24, 2022

As you can see on this image, Command classes have very poor coverage currently (16% of lines covered in total):

Edit: To see latest guidelines on what to work on see #10796 (comment)

Here some examples of how to write sane integration tests for most commands:

public function testCanListScripts(): void
{
$this->initTempComposer([
'scripts' => [
'test' => '@php test',
'fix-cs' => 'php-cs-fixer fix',
],
'scripts-descriptions' => [
'fix-cs' => 'Run the codestyle fixer',
],
]);
$appTester = $this->getApplicationTester();
$appTester->run(['command' => 'run-script', '--list' => true]);
$appTester->assertCommandIsSuccessful();
$output = $appTester->getDisplay();
$this->assertStringContainsString('Runs the test script as defined in composer.json.', $output, 'The default description for the test script should be printed');
$this->assertStringContainsString('Run the codestyle fixer', $output, 'The custom description for the fix-cs script should be printed');
}

/**
* @dataProvider provideConfigUpdates
* @param array<mixed> $before
* @param array<mixed> $command
* @param array<mixed> $expected
*/
public function testConfigUpdates(array $before, array $command, array $expected): void
{
$this->initTempComposer($before);
$appTester = $this->getApplicationTester();
$appTester->run(array_merge(['command' => 'config'], $command));
$appTester->assertCommandIsSuccessful($appTester->getDisplay());
$this->assertSame($expected, json_decode((string) file_get_contents('composer.json'), true));
}
public function provideConfigUpdates(): \Generator
{
yield 'set scripts' => [
[],
['setting-key' => 'scripts.test', 'setting-value' => ['foo bar']],
['scripts' => ['test' => 'foo bar']],
];
yield 'unset scripts' => [
['scripts' => ['test' => 'foo bar', 'lala' => 'baz']],
['setting-key' => 'scripts.lala', '--unset' => true],
['scripts' => ['test' => 'foo bar']],
];

$this->initTempComposer($composerJson);
$packages = [
$this->getPackage('first/pkg', '2.3.4'),
$this->getPackage('second/pkg', '3.4.0'),
];
$devPackages = [
$this->getPackage('dev/pkg', '2.3.4.5'),
];
$this->createInstalledJson($packages, $devPackages);
if ($lock) {
$this->createComposerLock($packages, $devPackages);
}
$appTester = $this->getApplicationTester();
$appTester->run(array_merge(['command' => 'bump'], $command));
$json = new JsonFile('./composer.json');
$this->assertSame($expected, $json->read());

If you feel like helping, PRs are very much welcome, I don't think we absolutely need 100% coverage but having the most common use cases covered would already be very valuable. Please target the main branch if you want to contribute some, and feel free to announce it here if you start some larger chunk of work to avoid duplicating efforts.

@theoboldalex
Copy link
Contributor

@Seldaek I'd love to take on some of this work. I have opened a first PR here #10932 and will start to work my way through the commands.

@Seldaek
Copy link
Member Author

Seldaek commented Jul 9, 2022

@theoboldalex great, thanks! Maybe wait until I get a chance to review the first one to make sure you don't run off and do more of them with the wrong assumptions. I'll try to get to it soon.

Also note the listing above is a bit outdated as I tried to work on this here and there when touching commands the last few months. But there is still plenty to be done :)

@Seldaek Seldaek modified the milestones: 2.4, 2.5 Jul 17, 2022
@ralflang
Copy link
Contributor

No promises but I may make the occassional PR every now and then.
Might be a good learning exercise. I have been playing around with the Command, Plugin, Capable ... parts of the codebase last month.

@Seldaek
Copy link
Member Author

Seldaek commented Aug 20, 2022

Update, we're at 36% (up from 16!) already now:

image

@tomekpryjma
Copy link
Contributor

I'd like to help out with this now and again - is there a way to see the current coverage?

greew added a commit to greew/composer that referenced this issue Oct 5, 2022
I've added tests for the BumpCommand to increase the test coverage.

See composer#10796

Signed-off-by: Jesper Skytte <jesper@skytte.it>
@Seldaek
Copy link
Member Author

Seldaek commented Oct 12, 2022

@tomekpryjma nope sorry, gotta run the tests with --coverage-html foo yourself then check in foo/index.html for the output.

Seldaek pushed a commit that referenced this issue Oct 13, 2022
I've added tests for the BumpCommand to increase the test coverage.

See #10796

Signed-off-by: Jesper Skytte <jesper@skytte.it>

Signed-off-by: Jesper Skytte <jesper@skytte.it>
Seldaek pushed a commit that referenced this issue Oct 13, 2022
@Seldaek
Copy link
Member Author

Seldaek commented Oct 13, 2022

Now at 41%! Here are the classes still needing the most effort IMO:

  • BaseDependencyCommand.php (needs to be tested via why/why-not commands)
  • CheckPlatformReqsCommand.php (WIP Added CheckPlatformReqsCommand Test #11079)
  • FundCommand.php
  • HomeCommand.php (WIP Test home command #11254 probably only can test with the --show flag enabled)
  • InstallCommand.php
  • SuggestsCommand.php (WIP Tests for Audit/Remove/Suggests Commands #11162)
  • ValidateCommand.php
  • RemoveCommand.php
  • UpdateCommand.php with --interactive flag (needs to run the command in interactive mode to test the uncovered bits, and use $applicationTester->setInputs() to configure/fake the user inputs)
  • InitCommand.php & PackageDiscoveryTrait.php (needs to run the command in interactive mode to test the uncovered bits, and use $applicationTester->setInputs() to configure/fake the user inputs)
  • ShowCommand.php (tested quite a bit but still only 40% covered as it is a large one)
  • GlobalCommand.php (might be tricky to test as it requires some internal knowledge)

These need an install to be run to actual get a working vendor dir, using packages with type metapackage helps making them installable without actual source:

  • StatusCommand.php
  • ReinstallCommand.php

@giulio-Joshi
Copy link
Contributor

Is this issue still relevant?

@Seldaek
Copy link
Member Author

Seldaek commented Oct 25, 2022

@giulio-Joshi yes for sure, not much happened since my last comment above yours.

@giulio-Joshi
Copy link
Contributor

Thanks for your answer.
I was planning on adding some more tests in the area, but paused the activity since my related PR had no feedback.
I was wondering if there was something wrong about it.

@Seldaek
Copy link
Member Author

Seldaek commented Oct 25, 2022

No it's just been 8 days, I'll get to it :)

@yesdevnull
Copy link
Contributor

yesdevnull commented Oct 28, 2022

Putting my hand up to work on tests for SuggestsCommand. Will submit a PR with my progress in the next ~24 hours, I currently have 100% for that command but I want to finesse the tests so they're not just chasing coverage.

PR created here: #11162.

@siims-biz
Copy link

@Seldaek What kind of skill/talent/tools are required/useful to contribute with testing?

@Seldaek
Copy link
Member Author

Seldaek commented Apr 9, 2023

@siims-biz knowledge of PHPUnit, some knowledge of composer (from user perspective good, internals better), the easy ones have been covered already though so if you aren't super familiar with all this I would say maybe rather find something else to work on, but up to you :)

@stof
Copy link
Contributor

stof commented Apr 14, 2023

Now at 52%

@theoboldalex
Copy link
Contributor

I'm going to make a start on testing ReinstallCommand tonight and will try to pick up StatusCommand after that if time permits.

theoboldalex added a commit to theoboldalex/composer that referenced this issue Jun 9, 2023
theoboldalex added a commit to theoboldalex/composer that referenced this issue Jun 9, 2023
theoboldalex added a commit to theoboldalex/composer that referenced this issue Jun 10, 2023
@Seldaek
Copy link
Member Author

Seldaek commented Jun 23, 2023

54.4% now, thanks everyone involved so far!

image

Here are the classes still needing the most effort, but overall we are really in a much better spot now I would say with the most critical commands pretty well covered:

  • BaseDependencyCommand.php (needs to be tested via why/why-not commands)
  • UpdateCommand.php with --interactive flag (needs to run the command in interactive mode to test the uncovered bits, and use $applicationTester->setInputs() to configure/fake the user inputs)
  • InitCommand.php & PackageDiscoveryTrait.php (needs to run the command in interactive mode to test the uncovered bits, and use $applicationTester->setInputs() to configure/fake the user inputs)
  • ShowCommand.php (tested quite a bit but still only 40% covered as it is a large one)
  • GlobalCommand.php (might be tricky to test as it requires some internal knowledge)

These need an install to be run to actual get a working vendor dir, using packages with type metapackage helps making them installable without actual source:

  • StatusCommand.php

theoboldalex added a commit to theoboldalex/composer that referenced this issue Jun 24, 2023
@rad8329
Copy link
Sponsor Contributor

rad8329 commented Jul 7, 2023

Will take test cases for commands why and why-not

@rad8329
Copy link
Sponsor Contributor

rad8329 commented Jul 25, 2023

Hi @Seldaek, I'd love to continue working on the above checklist, but I'm still waiting for Tests for base dependency command #11547 feedback, once I've received first feedback I could replicate that same approach in my upcoming PRs, thanks

@rad8329
Copy link
Sponsor Contributor

rad8329 commented Aug 20, 2023

Will try to complete show command test coverage, currently it is 55%

@stof
Copy link
Contributor

stof commented Aug 29, 2023

@Seldaek this should be re-opened. The PR #11547 contributed to this by adding more tests, but we are still not done yet.

@Seldaek Seldaek reopened this Aug 29, 2023
@stof
Copy link
Contributor

stof commented Aug 29, 2023

now at 58.11%:
image

@greew
Copy link
Contributor

greew commented Oct 5, 2023

FYI - I'm writing a lot of test for ShowCommand :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

11 participants