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

Update GitHubRepositories Tests and Refactor Get-GitHubRepository Function #233

Merged
merged 10 commits into from
Jun 28, 2020
Merged

Update GitHubRepositories Tests and Refactor Get-GitHubRepository Function #233

merged 10 commits into from
Jun 28, 2020

Conversation

X-Guardian
Copy link
Contributor

@X-Guardian X-Guardian commented Jun 12, 2020

Description

This PR updates the Pester tests for the GitHubRepositories module to increase the code coverage. This then exposed a code path in the Get-GithubRepository that could never be taken, so the function was refactored to remove this path and simplify the function logic.

A missing ValidateSet was also added to the Affiliation parameter of the Get-GitHubRepository function and incorrect Comment Based Help descriptions for the Get-GitHubRepositoryCollaborator and Move-GitHubRepositoryOwnership functions were fixed.

Issues Fixed

None

References

N/A

Checklist

  • You actually ran the code that you just wrote, especially if you did just "one last quick change".
  • Comment-based help added/updated, including examples.
  • Static analysis is reporting back clean.
  • New/changed code adheres to our coding guidelines.
  • Changes to the manifest file follow the manifest guidance.
  • Unit tests were added/updated and are all passing. See testing guidelines.
  • Relevant usage examples have been added/updated in USAGE.md.
  • If desired, ensure your name is added to our Contributors list

@HowardWolosky
Copy link
Member

Thanks for this. Conflicts abound in the tests between this and the work going on for pipelining (which is nearly completed), so I'm going to hold off on this one until early next week if you don't mind....

@HowardWolosky HowardWolosky added the bug This relates to a bug in the existing module. label Jun 12, 2020
@X-Guardian
Copy link
Contributor Author

Yep no problem Howard. I'll leave it in Draft state for now.

@HowardWolosky HowardWolosky added the api-repositories Work to complete the API's defined here: https://developer.github.com/v3/repos/ label Jun 18, 2020
@X-Guardian X-Guardian marked this pull request as ready for review June 20, 2020 10:56
Copy link
Member

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

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

Thanks so much for doing this. Increased code coverage in tests is always good, especially if it catches a dead block of code.

Looks like there are a few minor formatting issues and one bug introduced by this.

Otherwise, there may be some duplicative tests within here (especially around topics, collaborators and contributors) with the advent of the tests added for #242.

GitHubRepositories.ps1 Outdated Show resolved Hide resolved
GitHubRepositories.ps1 Outdated Show resolved Hide resolved
GitHubRepositories.ps1 Show resolved Hide resolved
GitHubRepositories.ps1 Show resolved Hide resolved
Tests/GitHubRepositories.tests.ps1 Outdated Show resolved Hide resolved
@HowardWolosky HowardWolosky added the waiting for update Waiting for an update to the PR before the next review label Jun 20, 2020
@X-Guardian
Copy link
Contributor Author

Can we trigger the CI for this PR?

Copy link
Member

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

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

Update looks great -- we just need to resolve the duplicative tests.

Tests/GitHubRepositories.tests.ps1 Show resolved Hide resolved
Tests/GitHubRepositories.tests.ps1 Show resolved Hide resolved
@X-Guardian
Copy link
Contributor Author

@HowardWolosky Can we trigger the CI for this PR?

@HowardWolosky
Copy link
Member

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Tests/GitHubRepositories.tests.ps1 Outdated Show resolved Hide resolved
Tests/GitHubRepositories.tests.ps1 Outdated Show resolved Hide resolved
Tests/GitHubRepositories.tests.ps1 Outdated Show resolved Hide resolved
Tests/GitHubRepositories.tests.ps1 Outdated Show resolved Hide resolved
Tests/GitHubRepositories.tests.ps1 Show resolved Hide resolved
Tests/GitHubRepositories.tests.ps1 Outdated Show resolved Hide resolved
HowardWolosky
HowardWolosky previously approved these changes Jun 27, 2020
@HowardWolosky
Copy link
Member

Now that #205 is in, you have some expected merge conflicts to deal with.

@HowardWolosky
Copy link
Member

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@HowardWolosky HowardWolosky merged commit eedfaa3 into microsoft:master Jun 28, 2020
@HowardWolosky HowardWolosky removed the waiting for update Waiting for an update to the PR before the next review label Jun 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-repositories Work to complete the API's defined here: https://developer.github.com/v3/repos/ bug This relates to a bug in the existing module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants