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

Pre push testsuites #1136

wants to merge 2 commits into from

Conversation

saidatom
Copy link

@saidatom saidatom commented May 18, 2024

Q A
Branch master for features and deprecations
Bug fix? no
New feature? yes
Documented? yes
Fixed tickets #1135

It is an experiment to get the #1135 working, actually there is the problem that if we have a grumphp.yml like that:

grumphp:
  process_timeout: 480
  tasks:
    phpcs:
      standard: PSR2
      ignore_patterns:
        - "spec/*Spec.php"
        - "test/*.php"
        - "stubs/*.php"
    phpspec:
      format: progress
      verbose: true
  testsuites:
    git_pre_push:
      tasks: [phpcs, phpspec]
  environment:
    paths:
      - tools

New Task Checklist:

  • Is the doc/tasks.md file updated?
  • Is the task registered in the tasks.yml file?
  • Does the task contains phpunit tests?
  • Does the task run in the correct context?
  • Is the run() method readable?
  • Is the run() method using the configuration correctly?
  • Are all CI services returning green?

Copy link
Contributor

@veewee veewee left a comment

Choose a reason for hiding this comment

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

Thanks @saidatom,

I've quickly skimmed through the code and added some pointers / questions.
Beside the review comments, A lot of tests are missing at the moment.

/**
* @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

{
$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.


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.

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?

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?

@@ -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?

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?

@@ -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?

/**
* @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.

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 ?

@veewee veewee mentioned this pull request May 22, 2024
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants