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

Pre push testsuites #1136

Open
wants to merge 2 commits into
base: v2.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ Since GrumPHP is just a CLI tool, these commands can be triggered:
- [git:deinit](doc/commands.md#installation)
- [git:pre-commit](doc/commands.md#git-hooks)
- [git:commit-msg](doc/commands.md#git-hooks)
- [git:pre-push](doc/commands.md#git-hooks)
- [run](doc/commands.md#run)

## Compatibility
Expand Down
2 changes: 1 addition & 1 deletion doc/commands.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ GrumPHP will be triggered with GIT hooks. However, you can run following command
```sh
php ./vendor/bin/grumphp git:pre-commit
php ./vendor/bin/grumphp git:commit-msg
php ./vendor/bin/grumphp git:pre-push
```

Both commands support raw git diffs and file lists as STDIN input.
Expand Down Expand Up @@ -76,4 +77,3 @@ git ls-files src | php ./vendor/bin/grumphp run
```

:exclamation: *If you use the stdin, we won't be able to answer questions interactively.*

3 changes: 3 additions & 0 deletions doc/testsuites.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,4 +44,7 @@ grumphp:
# Specify the test-suite for the git:pre-commit command:
git_pre_commit:
tasks: []
# Specify the test-suite for the git:pre-push command:
git_pre_push:
tasks: []
```
2 changes: 2 additions & 0 deletions grumphp.yml.dist
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ grumphp:
testsuites:
git_pre_commit:
tasks: [phpcs, phpspec, phpunit, composer, composer_normalize, yamllint, phplint, phpparser, psalm]
git_pre_push:
tasks: [phpcs, phpspec, phpunit, composer, composer_normalize, yamllint, phplint, phpparser, psalm]
# On CI, we run paratest separately. For some reason this currently fails in GitHub actions.
ci:
tasks: [phpcs, phpspec, phpunit, composer, composer_normalize, yamllint, phplint, phpparser, psalm]
Expand Down
9 changes: 9 additions & 0 deletions resources/config/console.yml
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,15 @@ services:
- '@GrumPHP\IO\IOInterface'
tags:
- { name: 'console.command' }
GrumPHP\Console\Command\Git\PrePushCommand:
arguments:
- '@GrumPHP\Collection\TestSuiteCollection'
- '@GrumPHP\Locator\StdInFiles'
- '@GrumPHP\Locator\ChangedFiles'
- '@GrumPHP\Runner\TaskRunner'
- '@GrumPHP\IO\IOInterface'
tags:
- { name: 'console.command' }

# This one is loaded through the TestSuiteCompilerPass
GrumPHP\Collection\TestSuiteCollection:
Expand Down
Binary file added resources/hooks/.DS_Store
Binary file not shown.
6 changes: 6 additions & 0 deletions resources/hooks/local/pre-push
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#!/bin/sh

$(ENV)

# Run GrumPHP
(cd "${HOOK_EXEC_PATH}" | $(EXEC_GRUMPHP_COMMAND) $(HOOK_COMMAND))
13 changes: 13 additions & 0 deletions resources/hooks/vagrant/pre-push
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#!/bin/sh

$(ENV)

# Run GrumPHP
cd $(VAGRANT_HOST_DIR) && vagrant ssh --command '$(which sh)' << COMMANDS
cd $(VAGRANT_PROJECT_DIR)

# Add grumphp envs:
$(ENV)

$(EXEC_GRUMPHP_COMMAND) $(HOOK_COMMAND))
COMMANDS
115 changes: 115 additions & 0 deletions src/Console/Command/Git/PrePushCommand.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
<?php

declare(strict_types=1);

namespace GrumPHP\Console\Command\Git;

use GrumPHP\Collection\FilesCollection;
use GrumPHP\Collection\TestSuiteCollection;
use GrumPHP\IO\IOFactory;
use GrumPHP\IO\IOInterface;
use GrumPHP\Locator\ChangedFiles;
use GrumPHP\Locator\StdInFiles;
use GrumPHP\Runner\TaskRunner;
use GrumPHP\Runner\TaskRunnerContext;
use GrumPHP\Task\Context\GitPrePushContext;
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Input\InputOption;
use Symfony\Component\Console\Output\OutputInterface;

/**
* This command runs the git pre-push hook.
*/
class PrePushCommand extends Command
{
const COMMAND_NAME = 'git:pre-push';
const EXIT_CODE_OK = 0;
const EXIT_CODE_NOK = 1;

/**
* @var TestSuiteCollection
*/
private $testSuites;

/**
* @var StdInFiles
*/
private $stdInFilesLocator;
Copy link
Contributor

Choose a reason for hiding this comment

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

The STDIN files locator is not being used


/**
* @var ChangedFiles
*/
private $changedFilesLocator;

/**
* @var TaskRunner
*/
private $taskRunner;

private IOInterface $io;

public function __construct(
TestSuiteCollection $testSuites,
StdInFiles $stdInFilesLocator,
ChangedFiles $changedFilesLocator,
TaskRunner $taskRunner,
IOInterface $io
) {
parent::__construct();

$this->testSuites = $testSuites;
$this->stdInFilesLocator = $stdInFilesLocator;
$this->changedFilesLocator = $changedFilesLocator;
$this->taskRunner = $taskRunner;
$this->io = $io;
}

public static function getDefaultName(): string
{
return self::COMMAND_NAME;
}

protected function configure(): void
{
$this->setDescription('Executed by the pre-push hook');
$this->addOption(
'skip-success-output',
Copy link
Contributor

Choose a reason for hiding this comment

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

should output be ever skipped?
For pre-commit, this is the case because the commit-msg runs in the same git commit command, just behind the other hook.

null,
InputOption::VALUE_NONE,
'Skips the success output. This will be shown by another command in the git commit hook chain.'
);
}

public function execute(InputInterface $input, OutputInterface $output): int
{
$localRef = $this->changedFilesLocator->getLocalRef();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to introduce a new PushedFiles locator which contains all of this logic and the getPushedFiles() logic. Pollution the ChangedFiles locator doesn't seem like a good idea here.
By moving it to its own locator, you can keep these specific git details "hidden" and just use the pushed files from this command.

I'm not sure how it behaves exactly, but I'm sure test-cases will be added to show how it works exactly.

$localSha = $this->changedFilesLocator->getLocalSHA1();
$remoteSha = $this->changedFilesLocator->getRemoteSHA1();

$taskRunnerContext = (
new TaskRunnerContext(
new GitPrePushContext($this->getPushedFiles($localRef, $localSha, $remoteSha)),
$this->testSuites->getOptional('git_pre_push')
)
)->withSkippedSuccessOutput((bool) $input->getOption('skip-success-output'));

$output->writeln('<fg=yellow>GrumPHP detected a pre-push command.</fg=yellow>');
$results = $this->taskRunner->run($taskRunnerContext);

return $results->isFailed() ? self::EXIT_CODE_NOK : self::EXIT_CODE_OK;
}

protected function getPushedFiles(?string $localRef, ?string $localSha, ?string $remoteSha): FilesCollection
{
if (empty($remoteSha)) {
return $this->changedFilesLocator->locateFromRawDiffInput('HEAD');
}

if ($localRef === '(delete)') {
Copy link
Contributor

Choose a reason for hiding this comment

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

should the task run if you delete a branch ?

return new FilesCollection([]);
}

return $this->changedFilesLocator->locateFromGitDiff($remoteSha, $localSha);
}
}
64 changes: 62 additions & 2 deletions src/Locator/ChangedFiles.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
use GrumPHP\Git\GitRepository;
use GrumPHP\Util\Filesystem;
use GrumPHP\Util\Paths;
use Symfony\Component\Finder\SplFileInfo;
use SplFileInfo;

/**
* Class Git.
Expand Down Expand Up @@ -46,6 +46,66 @@ public function locateFromGitRepository(): FilesCollection
return $this->parseFilesFromDiff($diff);
}

public function getLocalRef(): ?string
{
return $this->repository->run('symbolic-ref', ['HEAD']);
}
public function getLocalSHA1(): ?string
{
return $this->repository->run('rev-parse', ['HEAD']);
}
private function getRemoteRef(): ?string
{
return $this->repository->tryToRunWithFallback(
function (): ?string {
return $this->repository->run('rev-parse', ['--abbrev-ref', '--symbolic-full-name', '@{u}']);
},
'null'
Copy link
Contributor

Choose a reason for hiding this comment

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

having a 'null' string doesn't seem like a good idea. Instead maybe use null?

);
}

public function getRemoteSHA1(): ?string
{
if ($this->getRemoteRef() === 'null') {
return null;
}
/**
* @psalm-suppress PossiblyUndefinedArrayOffset
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe better to validate the format and return null if it's unexpected?

* @psalm-suppress PossiblyNullArgument
*/
list($remoteName, $branchName) = explode('/', $this->getRemoteRef(), 2);
$output = $this->repository->run('ls-remote', [$remoteName, $branchName]);
return $output ? strtok($output, "\t") : null;
}

/**
* @return FilesCollection
*/
public function locateFromGitDiff(string $fromSha, ?string $toSha): FilesCollection
Copy link
Contributor

Choose a reason for hiding this comment

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

What worries me a bit, is that there seem to be a lot of different states possible which require slightly different approaches / solutions.
How sure are you that all possible situations are covered?

Maybe the new PushedFiled locator should also contain details on how it works and what it covers exactly.

{
if (empty($toSha)) {
$toSha = 'HEAD';
}
$files = [];
$commits = $fromSha . '..' . $toSha;
$diff = $this->repository->tryToRunWithFallback(
function () use ($commits): ?string {
return $this->repository->run('diff', ['--name-only', trim($commits), '--oneline']);
},
'null'
);
if ($diff === 'null') {
return new FilesCollection($files);
}

foreach (explode("\n", $diff) as $file) {
Copy link
Contributor

Choose a reason for hiding this comment

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

would this always be \n or \r\n dependeing on the system?

$fileObject = new SplFileInfo($file);
$files[] = $fileObject;
}

return new FilesCollection($files);
}

public function locateFromRawDiffInput(string $rawDiff): FilesCollection
{
$diff = $this->repository->createRawDiff($rawDiff);
Expand Down Expand Up @@ -75,6 +135,6 @@ private function makeFileRelativeToProjectDir(File $file): SplFileInfo
$file->isRename() ? $file->getNewName() : $file->getName()
);

return new SplFileInfo($filePath, dirname($filePath), $filePath);
return new SplFileInfo($filePath);
Copy link
Contributor

Choose a reason for hiding this comment

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

any specific reason why this was changed?

}
}
5 changes: 4 additions & 1 deletion src/Task/Ant.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use GrumPHP\Task\Config\ConfigOptionsResolver;
use GrumPHP\Task\Context\ContextInterface;
use GrumPHP\Task\Context\GitPreCommitContext;
use GrumPHP\Task\Context\GitPrePushContext;
use GrumPHP\Task\Context\RunContext;
use Symfony\Component\OptionsResolver\OptionsResolver;

Expand All @@ -36,7 +37,9 @@ public static function getConfigurableOptions(): ConfigOptionsResolver

public function canRunInContext(ContextInterface $context): bool
{
return $context instanceof GitPreCommitContext || $context instanceof RunContext;
return $context instanceof GitPreCommitContext
Copy link
Contributor

Choose a reason for hiding this comment

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

By default, this will run the task in both pre-commit and pre-push scenario?
I think that might be a bit too much for a lot of people.

Would it make sense to have a new metadata field that can be used to change the context instead?
See https://github.com/phpro/grumphp/blob/v2.x/doc/tasks.md#metadata

Something like:

grumphp:
    tasks:
        anytask:
            metadata:
                git_stage: ["pre-commit", "pre-push"]

Which can be used to specify at what stages in your git flow you want to run this task.
The canRunInContext will then by based on the metadata configuration, which will default to ["pre-commit"].

WDYT?

|| $context instanceof GitPrePushContext
|| $context instanceof RunContext;
}

public function run(ContextInterface $context): TaskResultInterface
Expand Down
5 changes: 4 additions & 1 deletion src/Task/Atoum.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use GrumPHP\Task\Config\ConfigOptionsResolver;
use GrumPHP\Task\Context\ContextInterface;
use GrumPHP\Task\Context\GitPreCommitContext;
use GrumPHP\Task\Context\GitPrePushContext;
use GrumPHP\Task\Context\RunContext;
use Symfony\Component\OptionsResolver\OptionsResolver;

Expand Down Expand Up @@ -47,7 +48,9 @@ public static function getConfigurableOptions(): ConfigOptionsResolver
*/
public function canRunInContext(ContextInterface $context): bool
{
return $context instanceof GitPreCommitContext || $context instanceof RunContext;
return $context instanceof GitPreCommitContext
|| $context instanceof GitPrePushContext
|| $context instanceof RunContext;
}

/**
Expand Down
5 changes: 4 additions & 1 deletion src/Task/Behat.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use GrumPHP\Task\Config\ConfigOptionsResolver;
use GrumPHP\Task\Context\ContextInterface;
use GrumPHP\Task\Context\GitPreCommitContext;
use GrumPHP\Task\Context\GitPrePushContext;
use GrumPHP\Task\Context\RunContext;
use Symfony\Component\OptionsResolver\OptionsResolver;

Expand Down Expand Up @@ -43,7 +44,9 @@ public static function getConfigurableOptions(): ConfigOptionsResolver
*/
public function canRunInContext(ContextInterface $context): bool
{
return $context instanceof GitPreCommitContext || $context instanceof RunContext;
return $context instanceof GitPreCommitContext
|| $context instanceof GitPrePushContext
|| $context instanceof RunContext;
}

/**
Expand Down
5 changes: 4 additions & 1 deletion src/Task/Brunch.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use GrumPHP\Task\Config\ConfigOptionsResolver;
use GrumPHP\Task\Context\ContextInterface;
use GrumPHP\Task\Context\GitPreCommitContext;
use GrumPHP\Task\Context\GitPrePushContext;
use GrumPHP\Task\Context\RunContext;
use Symfony\Component\OptionsResolver\OptionsResolver;

Expand Down Expand Up @@ -43,7 +44,9 @@ public static function getConfigurableOptions(): ConfigOptionsResolver
*/
public function canRunInContext(ContextInterface $context): bool
{
return $context instanceof GitPreCommitContext || $context instanceof RunContext;
return $context instanceof GitPreCommitContext
|| $context instanceof GitPrePushContext
|| $context instanceof RunContext;
}

/**
Expand Down
5 changes: 4 additions & 1 deletion src/Task/CloverCoverage.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use GrumPHP\Task\Config\TaskConfigInterface;
use GrumPHP\Task\Context\ContextInterface;
use GrumPHP\Task\Context\GitPreCommitContext;
use GrumPHP\Task\Context\GitPrePushContext;
use GrumPHP\Task\Context\RunContext;
use GrumPHP\Util\Filesystem;
use SimpleXMLElement;
Expand Down Expand Up @@ -72,7 +73,9 @@ public static function getConfigurableOptions(): ConfigOptionsResolver
*/
public function canRunInContext(ContextInterface $context): bool
{
return $context instanceof GitPreCommitContext || $context instanceof RunContext;
return $context instanceof GitPreCommitContext
|| $context instanceof GitPrePushContext
|| $context instanceof RunContext;
}

/**
Expand Down
5 changes: 4 additions & 1 deletion src/Task/Codeception.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use GrumPHP\Task\Config\ConfigOptionsResolver;
use GrumPHP\Task\Context\ContextInterface;
use GrumPHP\Task\Context\GitPreCommitContext;
use GrumPHP\Task\Context\GitPrePushContext;
use GrumPHP\Task\Context\RunContext;
use Symfony\Component\OptionsResolver\OptionsResolver;

Expand Down Expand Up @@ -45,7 +46,9 @@ public static function getConfigurableOptions(): ConfigOptionsResolver
*/
public function canRunInContext(ContextInterface $context): bool
{
return $context instanceof GitPreCommitContext || $context instanceof RunContext;
return $context instanceof GitPreCommitContext
|| $context instanceof GitPrePushContext
|| $context instanceof RunContext;
}

/**
Expand Down
Loading