[Fixes #731] Add unit tests for ReflectedActionDescriptorProvider #1087
Conversation
| @@ -25,7 +25,7 @@ public void GetDescriptors_GetsDescriptorsOnlyForValidActions() | |||
| var actionNames = descriptors.Select(ad => ad.Name); | |||
|
|
|||
| // Assert | |||
| Assert.Equal(new[] { "GetPerson", "ListPeople", }, actionNames); | |||
| Assert.Equal(new[] { "GetPerson", "ShowPeople", }, actionNames); | |||
There was a problem hiding this comment.
This test changed because I added [ActionName] to one of the actions to test that it gets taken into account
There was a problem hiding this comment.
This seems ok in this specific case, just be careful not to change the meaning of tests. Generally want we want to add new test data if you're adding tests.
|
test adds/refactoring looks good |
|
Any functional tests that should be added? |
|
From what I believe, this task meant to add unit tests to cover the rest of the aspects of this class, test coverage in functional tests should be good already (and not part of this task anyway). I haven't specifically looked at functional tests, but this should have good coverage as part of the coverage for other features. |
|
@rynowak Updated, I've addressed everything except whether the comments should be on the call site or inside the method. I don't think we have clear guidance for it and my usual take on it is that I prefer to have the why comments along with the place where I call them so that why and what are clear when I read the code and how (the body of the method) becomes an implementation detail. With that said, if you think different and have reasons to do it differently I'm happy to change it, I don't have a strong opinion on it. |
|
@javiercn - the updated code looks good. The difference is subtle and about the level of abstraction. A method call is necessarily at a higher-level of abstraction than the code inside the body.
TLDR follow your heart. We all have different thresholds for when we think 1) or 2) above needs a comment, and code reviews help with that. |
|
|
1. Added tests that cover parameters in actions. 2. Added tests that cover building the reflected application model. 3. Added tests that cover attribute routed action constraints and default values. 4. Added tests that cover conventionally routed action constraints and default values. 5. Refactored and cleaned up ReflectedActionDescriptorProvider. All the refactors consist of extracting blocks of code to separate methods to better display the flow when building the action descriptors.
of extracting blocks of code to separate methods to better display the flow when building
the action descriptors.