Skip to content
Snippets Groups Projects

Reroll patch #2 against 2.x branch.

Merged Bhanu D requested to merge issue/tfa-3381701:3381701-provide-drush-command into 2.x
All threads resolved!

Closes #3381701

Merge request reports

Merge train pipeline #52551 passed

Merge train pipeline passed for c61ba4cf

Test coverage 24.54% (-0.31%) from 1 job
Code Quality is loading
Test summary results are being parsed

Merged by Conrad LaraConrad Lara 1 year ago (Nov 20, 2023 6:41am UTC)

Loading

Pipeline #52559 passed

Pipeline passed for c61ba4cf on 2.x

Test coverage 24.54% (-0.31%) from 1 job

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • This will still need unit tests.

  • Bhanu D added 1 commit

    added 1 commit

    • ee7b0f97 - Apply 4 suggestion(s) to 1 file(s)

    Compare with previous version

  • Bhanu D added 1 commit

    added 1 commit

    Compare with previous version

  • Conrad Lara resolved all threads

    resolved all threads

  • Overall looks good, Unit tests will be the next step to get this in.

    The TfaUserDataTrait PHPStan errors will not be fixed in this issue as they represent faults in the trait itself not the added code.

    We will need to add them to the baseline before final merge.

  • Author Contributor

    @cmlara do you have any reference for unit testing of drush commands ? or functional test like this is fine ?

  • Conrad Lara added 1 commit

    added 1 commit

    • 1ccb09a2 - Move command logic to a Drush service and add unit testing.

    Compare with previous version

  • I would prefer unit. That said, your question made me realize I haven't actually converted any other project's functional Drush test Unit and that I really needed to at least look at doing one myself before I put the burden on others.

    After a brief discussion in the Drush slack it turns out that there is no good/easy way to mock $this->io(). This necessitates moving the new commands out into a service where we can inject the io.

    I've gone ahead and moved the code you have written to a new Drush service. This was mostly a straight copy with a couple of changes to refer to different services values. I was also able to add back in the strict_type as it is a new file. I've also added the tests.

    Note: I want to make it clear this reworking isn't intended to be an indicator you did anything wrong or that I did not believe you could make these changes. I did the rework solely because I felt an obligation to understand what hurdles others may encounter with testing Drush commands Unit Testing and knowing that I'm responsible for not being aware of this earlier.

  • Author Contributor

    Note: I want to make it clear this reworking isn't intended to be an indicator you did anything wrong or that I did not believe you could make these changes.

    Hey, I get it No hard feelings. I saw your message in slack channel as well before you posting this here. If you have asked me to write Unit test I might not be able to do so as I am not familiar with this approach. Even if you have asked me to write functional test I would not be able to do so in last week due to my travel and little availability of time.

    I did the rework solely because I felt an obligation to understand what hurdles others may encounter with testing Drush commands Unit Testing and knowing that I'm responsible for not being aware of this earlier.

    I really appreciate you making your time to actually try to understand how complex it is before asking someone to go down the rabbit hole.

    Edited by Bhanu D
  • Conrad Lara added 2 commits

    added 2 commits

    • 35f4ab9d - Rename TokenManagement to TfaTokenManagement.
    • 0d261edf - Update baseline.

    Compare with previous version

  • Conrad Lara resolved all threads

    resolved all threads

  • Conrad Lara added 10 commits

    added 10 commits

    • 4b8074ed - 1 commit from branch project:2.x
    • 36b28f14 - Reroll patch #2 against 2.x branch.
    • 628d40e0 - Updated Code.
    • 604d3917 - Updated logger messages.
    • 796c8e94 - Updated return message.
    • 9b7e78d9 - Apply 4 suggestion(s) to 1 file(s)
    • ba8cf0d4 - Update TfaCommands.php
    • 84725578 - Move command logic to a Drush service and add unit testing.
    • 0b30fe6e - Rename TokenManagement to TfaTokenManagement.
    • cbfc7ee4 - Update baseline.

    Compare with previous version

  • Conrad Lara enabled automatic add to merge train when the pipeline for d670f136 succeeds

    enabled automatic add to merge train when the pipeline for d670f136 succeeds

  • Conrad Lara started a merge train

    started a merge train

  • merged

  • Please register or sign in to reply
    Loading