-
Notifications
You must be signed in to change notification settings - Fork 778
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
Porting old Umbraco API Controller [Unit Testing] #6524
Porting old Umbraco API Controller [Unit Testing] #6524
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @erikjanwestendorp ! 💪
Just a single comment from my side.
Co-authored-by: sofietoft <[email protected]>
@sofietoft Thanks for the review, just updated the PR. |
Thanks @erikjanwestendorp ! 💪 I've been testing this, but cannot get it to work 😅 Seems I need to setup a lot of things around it, in order to test properly. I'll pass this one on to our developers for a confirmation 💪 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had this past one of our developers, and it looks like the code in the test is using some legacy methods from the NUnit framework.
So, a few changes should be made here:
Line 200: Swap AreEqual
with That
Assert.That(expected == result);
Line 206: Change to JsonSerializer
var json = JsonSerializer.Serialize(this.controller.GetAllProductsJson().Value);
Line 208: Swap AreEqual
with That
Assert.That("[\"Table\",\"Chair\",\"Desk\",\"Computer\",\"Beer fridge\"]" == json);
I know this isn't related to what you're actually changing in this PR, but let's get this test fixed, then I'll create some issues for going through the rest of the tests, as they might also be using the legacy methods 💪
Hope it all makes sense 🤞
@sofietoft Thanks for the review just updated the PR with your suggestions (including the unit test) 😄 |
Looks great @erikjanwestendorp ! 👏 I'll make sure this is merged! |
Description
This PR belongs to: #6466.
Type of suggestion
Product & version (if relevant)
Umbraco CMS v14 & 15