Rangeselector tests #3926
Rangeselector tests #3926
Conversation
|
Thanks RosarioPulella for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌 |
ghost
requested review from
There was a problem hiding this comment.
@azchohfi I added this code to be able to use this syntax for defining a record for these tests. From what I read it seem like its not a big hack or anything, do you think its fine to add? It should only effect our testing code.
|
@azchohfi @Sergio0694 @michael-hawker @shweaver-MSFT @deltakosh @skendrot I am still looking to expand these tests, but I would like some feedback. Any thoughts about what behavior this shows in the |
|
I loved that you used the equality of records to Assert results, but I'm not sure about the benefits with this solution, versus the complexity. I'm assuming you did this just to make sure that the test is asserting only one thing, but I would be fine to assert each property individually just for the simplicity. Not sure what others think. I'm not saying I'm not ok with this implementation (and its workaround for records in dot net native), but I'm not sure if it is really a benefit here. You have the parameter values in the same method, and you could just assert them individually. I'm fine with either. |
I want to assert on the equality of all the properties together as a record so that if any of the values are not as expected then instead of seeing the just one value that is not expected we get to see the values of all the properties, giving use more information in CI without us having to expand the test manually or set break points. For this control its generally important because for many of the properties if you change one it could effect others. |
|
IMO, that is enough explanation to keep it this way then! Makes sense! |
|
I like the idea that a failure provides all the info about the case that's failing compared to a single value too. Let's snap this in if we think they're good, then work towards adding more cases or adjusting cases to what we want the desired behavior to be (especially around the snap points). Then we can use that to make modifications to the existing logic (as we know they're broken cases). |
|
In 9df2a34, I removed the |
|
@michael-hawker @azchohfi and @chingucoding mind looking at this, I would like to get this merged so we can move forward with discussing, addressing issues and adjusting behavior and around the |
allows non-verbose record decerlation syntax Fix for "error CS0518: Predefined type 'System.Runtime.CompilerServices.IsExternalInit' is not defined or imported" see https://stackoverflow.com/questions/64749385/predefined-type-system-runtime-compilerservices-isexternalinit-is-not-defined https://developercommunity.visualstudio.com/t/error-cs0518-predefined-type-systemruntimecompiler/1244809
It feels hard to maintain in a way that would be usefull
|
Hello @michael-hawker! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
4 participants
Fixes Adds tests for the range selector properties.
Added unit tests for code interactions with the range selector properties.
PR Type
What kind of change does this PR introduce?
What is the current behavior?
No tests for the RangeSelector properties.
What is the new behavior?
Tests for the RangeSelector properties.
PR Checklist
Please check if your PR fulfills the following requirements:
Other information