这是indexloc提供的服务，不要输入任何密码
Skip to content

Add support for phive-install. #354

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
Closed
wants to merge 1 commit into from
Closed

Add support for phive-install. #354

wants to merge 1 commit into from

Conversation

@mihai-stancu
Copy link
Contributor

@mihai-stancu mihai-stancu commented Feb 19, 2021

Hi,

I'm trying to add phive as an installer here. It's prepackaged to do what phar-download does and I've seen various tool starting to recommend it as "the way" to install their tool.

WDYT?

@jakzal
Copy link
Owner

jakzal commented Feb 20, 2021

Great idea. It should be prefered over phar-download 👍

@jakzal
Copy link
Owner

jakzal commented Feb 20, 2021 •
edited
Loading

To help with infection coverage you should add a test to the InstallToolsTest. Similar to the one for phar-download:

toolbox/tests/UseCase/InstallToolsTest.php

Lines 151 to 160 in 7112546

public function test_it_includes_phar_download_commands()
{
$this->tools->all(Argument::type(Filter::class))->willReturn(Collection::create([
$this->tool(new PharDownloadCommand('https://github.com/sensiolabs-de/deptrac/releases/download/0.2.0/deptrac-0.2.0.phar', '/tools/phar')),
]));
$command = $this->useCase->__invoke($this->filter());
$this->assertMatchesRegularExpression('#curl[^&]*?deptrac-0.2.0.phar#', (string)$command);
}

@mihai-stancu
Copy link
Contributor Author

mihai-stancu commented Feb 20, 2021

Question: (as a matter of test philosophy) since you (fully) control the input, then why use regex to match the output, why not match against the exact expected command string?

@jakzal
Copy link
Owner

jakzal commented Feb 22, 2021

Question: (as a matter of test philosophy) since you (fully) control the input, then why use regex to match the output, why not match against the exact expected command string?

If the test was checking for the exact output, it would be more coupled to the implementation. All I want to make sure here is that curl is called to download the phar. I don't care about specific options or the order. The actual command invocation is tested in the integrated test.

@mihai-stancu
Copy link
Contributor Author

mihai-stancu commented Feb 22, 2021

Alright, 10q for the clarification.

I'll pingback in a couple of days -- when I find the time to finish this.

@jakzal
Copy link
Owner

jakzal commented Feb 23, 2021

No rush! Thank you for working on this.

This was referenced Mar 3, 2021
Merged
Merged
@mihai-stancu
Copy link
Contributor Author

mihai-stancu commented Mar 3, 2021

I'll close this one since the integration tests here aren't revealing faults in the implementation and we'll rely on #369 to finish the job.

@mihai-stancu mihai-stancu deleted the phive branch March 3, 2021 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mihai-stancu @jakzal