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

Add tests to GitHubRepositories.tests.ps1 #176

Merged

Conversation

giuseppecampanelli
Copy link
Contributor

@giuseppecampanelli giuseppecampanelli commented May 21, 2020

Fixes #139
Add tests for GitHubRepositories.ps1 functions

Add tests for GitHubRepositories.ps1 functions
@HowardWolosky
Copy link
Member

Thanks, @themilanfan! I'll take a look at this over the weekend.

@HowardWolosky HowardWolosky self-assigned this May 22, 2020
@HowardWolosky HowardWolosky added the technical debt Work that was postponed by a previous change. label May 22, 2020
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 for the submission, @themilanfan! Much appreciated!

Are you sure that you ran this iteration of the tests? There are a number of issues with them that should have caused failures. I added notes for a lot of them, but then I stopped looking further.

Try merging in the most recent changes in master and then take a stab at updating these tests again and make sure that they all pass. It's possible that you have some local default configuration for DefaultOwnerName and DefaultRepositoryName which is confusing things, however Common.ps1 should have swapped that out with the default configuration during test execution.

I'll leave it to you to investigate these a bit more.

Looking forward to future updates.

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 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 Outdated Show resolved Hide resolved
Tests/GitHubRepositories.tests.ps1 Outdated Show resolved Hide resolved
Remove -Verbose, parenthesis, and s from Topics
@giuseppecampanelli
Copy link
Contributor Author

Pending merge of issue #174 once it's merged to master so I can add the missing -Confirm:$false

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 for the update, @themilanfan. It looks like there are still some issues to address. I didn't thoroughly look through this yet. I suggest that you run the tests all locally as well in a clean PowerShell session to try and see some of the issues that I'm point out.

Thanks again for all your hard work. Looking forward to your next update!

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 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 Outdated Show resolved Hide resolved
@giuseppecampanelli
Copy link
Contributor Author

@HowardWolosky Sorry I wasn't explicit, I plan on testing and fixing the kinks once #174 gets merged so I can include the missing -Confirm:$false. However, I will fix the other things you noticed in the meantime 😄 I'll make sure next time to be clear so I can help you avoid reviewing for nothing!

Added remaining -Confirm:$false following new merges that added -Confirm:$false functionality.
@giuseppecampanelli
Copy link
Contributor Author

[WIP] Pending testing

@giuseppecampanelli
Copy link
Contributor Author

giuseppecampanelli commented Jun 2, 2020

@HowardWolosky almost all is fixed, I'm getting one error for a test involving clearing repository topics. Could it be a bug?

When i ran the Set-GitHubRepositoryTopic -OwnerName $ownerName -RepositoryName $repoName -Clear outside of the test case, I got the following:

Exception: /Users/user/dev/github/PowerShellForGitHub/GitHubCore.ps1:322 Line | 322 | throw $remoteErrors[0].Exception | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | {"message":"Invalid request.\n\nFor 'items', nil is not a string.","documentation_url":"https://developer.github.com/v3/repos#replace-all-topics-for-a-repository"}

@X-Guardian
Copy link
Contributor

@themilanfan, I've raised issue #215 for this bug and PR #216 to fix it.

@giuseppecampanelli
Copy link
Contributor Author

@HowardWolosky should be good for review following issue raised & PR from @X-Guardian (thanks!).

@HowardWolosky
Copy link
Member

Finding that bug (#215) is a fantastic outcome to this PR. I love that by writing some missing UT's we identified an actual bug in the module, and that we now have a UT to catch further regressions of that area in the future. Thanks again!

@giuseppecampanelli
Copy link
Contributor Author

giuseppecampanelli commented Jun 3, 2020

@HowardWolosky can confirm all tests pass now since #216 was merged

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 for the great update.

Two minor fixes (one logic error, and one comment fix that I would have ignored if the logic error hadn't been there too) and this will be ready to go.

Tests/GitHubRepositories.tests.ps1 Outdated Show resolved Hide resolved
Tests/GitHubRepositories.tests.ps1 Outdated Show resolved Hide resolved
@giuseppecampanelli
Copy link
Contributor Author

Changes committed 👍 @HowardWolosky

@HowardWolosky
Copy link
Member

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

HowardWolosky
HowardWolosky previously approved these changes Jun 3, 2020
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.

Awesome. Running CI build now to verify, then will merge in. Thanks again!

@HowardWolosky HowardWolosky added the tests A change related to the Pester tests for the module. label Jun 3, 2020
@HowardWolosky
Copy link
Member

HowardWolosky commented Jun 3, 2020

Your CI build failed due to static analysis failures:

##[warning]Tests\GitHubRepositories.tests.ps1(129,17): Warning : The variable 'repo' is assigned but never used.
##[warning]Tests\GitHubRepositories.tests.ps1(154,17): Warning : The variable 'repo' is assigned but never used.
##[warning]Tests\GitHubRepositories.tests.ps1(158,17): Warning : The variable 'delete' is assigned but never used.
##[warning]Tests\GitHubRepositories.tests.ps1(170,17): Warning : The variable 'newRepoName' is assigned but never used.
##[warning]Tests\GitHubRepositories.tests.ps1(193,17): Warning : The variable 'repo' is assigned but never used.
##[warning]Tests\GitHubRepositories.tests.ps1(217,17): Warning : The variable 'repo' is assigned but never used.
##[warning]Tests\GitHubRepositories.tests.ps1(242,17): Warning : The variable 'repo' is assigned but never used.
##[warning]Tests\GitHubRepositories.tests.ps1(265,17): Warning : The variable 'repo' is assigned but never used.

Please verify static analysis is passing before submitting your next update. Thanks!

@giuseppecampanelli
Copy link
Contributor Author

@HowardWolosky apologies, will be more careful for next one. I've followed the same method other tests use to avoid this issue. Ready to re-run

@HowardWolosky
Copy link
Member

@HowardWolosky apologies, will be more careful for next one. I've followed the same method other tests use to avoid this issue. Ready to re-run

No worries. We had a bunch that had snuck in over time that I recently addressed (#180). That's why I got the CI build healthy again...to help keep master clean. And it looks like #193 also just failed for static analysis failures too, so you're in good company.

Will kick off a new CI build now.

@HowardWolosky
Copy link
Member

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@HowardWolosky
Copy link
Member

HowardWolosky commented Jun 3, 2020

There was one failing test, but only on macOS, and it was in Project Cards (unrelated) which means that there was something flakey going on.

Expected strings to be the same, but they were different.
Expected length: 11
Actual length: 16
Strings differ at index 8.
Expected: 'TestCardTwo'
But was: 'TestCard_Updated'

Stack trace
at <ScriptBlock>, /Users/runner/runners/2.169.1/work/1/s/Tests/GitHubProjectCards.tests.ps1: line 157
157:                 $results[1].note | Should be $defaultCardTwo

Merging this in now. Thanks for all your efforts! 🎆

@HowardWolosky HowardWolosky merged commit 742b853 into microsoft:master Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
technical debt Work that was postponed by a previous change. tests A change related to the Pester tests for the module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance UT's for GitHubRepositories.ps1
3 participants