KEMBAR78
Add XUnit test for `HandleDoubleAndSingleQuote` in CompletionHelpers class by ArmaanMcleod · Pull Request #25181 · PowerShell/PowerShell · GitHub
Skip to content

Conversation

@ArmaanMcleod
Copy link
Contributor

@ArmaanMcleod ArmaanMcleod commented Mar 15, 2025

PR Summary

So to make sure the HandleDoubleAndSingleQuote method in CompletionHelpers class is covered from regression, I have added tests to ensure code is covered by regressions.

PR Context

PR Checklist

Copy link
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

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

I suggest to add only TestHandleDoubleAndSingleQuote tests in the PR. I have many questions for rest.

@iSazonov iSazonov added CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log CL-Test Indicates that a PR should be marked as a test change in the Change Log labels Mar 15, 2025
@iSazonov iSazonov self-assigned this Mar 15, 2025
@ArmaanMcleod ArmaanMcleod changed the title Add XUnit test for all methods in CompletionHelpers class Add XUnit test for HandleDoubleAndSingleQuote in CompletionHelpers class Mar 15, 2025
@ArmaanMcleod
Copy link
Contributor Author

I suggest to add only TestHandleDoubleAndSingleQuote tests in the PR. I have many questions for rest.

All good @iSazonov. I have only included test for HandleDoubleAndSingleQuote. I think out of all methods this one is important to have tests because the logic itself is quite complicated and probably very easy to introduce regression 😄.

@ArmaanMcleod ArmaanMcleod requested a review from iSazonov March 15, 2025 12:47
@iSazonov iSazonov removed the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Mar 15, 2025
@iSazonov

This comment was marked as outdated.

@azure-pipelines

This comment was marked as outdated.

@iSazonov

This comment was marked as outdated.

@iSazonov
Copy link
Collaborator

I suggest to move tests for QuoteCompletionText and GetMatchingResults to other PRs. If we add QuoteCompletionText the tests should be in that PR.

@iSazonov
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@iSazonov
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@iSazonov iSazonov enabled auto-merge (squash) March 16, 2025 08:35
@ArmaanMcleod
Copy link
Contributor Author

@iSazonov Looks like unrelated packaging failures for Windows CI. Might need a rerun. Not sure though if this can auto merge with PSResourceGet and CodeFactor issues.

@iSazonov iSazonov merged commit e8bf560 into PowerShell:master Mar 17, 2025
37 of 40 checks passed
@microsoft-github-policy-service
Copy link
Contributor

microsoft-github-policy-service bot commented Mar 17, 2025

📣 Hey @ArmaanMcleod, how did we do? We would love to hear your feedback with the link below! 🗣️

🔗 https://aka.ms/PSRepoFeedback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-Test Indicates that a PR should be marked as a test change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants