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 more profile GUID tests #17030

Merged
merged 4 commits into from
Apr 9, 2024

Conversation

marcelwgn
Copy link
Contributor

Summary of the Pull Request

Adding more profile GUID tests

References and Relevant Issues

Closes #2119

Detailed Description of the Pull Request / Additional comments

Currently, there are formats that simply break GUID parsing (the commented out test cases). Should we catch that and treat it like a "null" GUID or should we generate a new GUID for those profiles?

Validation Steps Performed

PR Checklist

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Task It's a feature request, but it doesn't really need a major design. Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Product-Terminal The new Windows Terminal. labels Apr 8, 2024

This comment has been minimized.

This comment has been minimized.

VERIFY_IS_FALSE(profile2->HasGuid());
VERIFY_IS_TRUE(profile3->HasGuid());
VERIFY_IS_TRUE(profile4->HasGuid());
// The following crash JSON parsing
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, this should throw an exception which you can verify you caught with VERIFY_THROWS!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure if they should crash so I left them as comment, now they are using VERIFY_THROWS (thanks for mentioning that!)

Copy link
Member

@DHowett DHowett 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!

Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

This is great! Thanks!

@carlos-zamora carlos-zamora added this pull request to the merge queue Apr 9, 2024
Merged via the queue into microsoft:main with commit 7adc374 Apr 9, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add some tests for invalid GUIDs in JSON
3 participants