Add some tests of Swagger.ObjectModel #128
Conversation
merge some new commits
…s Utilities class.
|
Hm. I need to work on the build. It seems Nancy.Swagger.Annotations hasn't been pushed to NuGet in a while. But the build didn't claim to fail. (Unrelated to your PR, but just posting because I noticed it on yours. But I'll also need to make sure the build fails if the test fails.) |
| } | ||
|
|
||
| [Fact] | ||
| public void Should_ThrowRequiredFieldException_WhenUrlIsNullOrEmpty() |
There was a problem hiding this comment.
Yeah, the tests are not all following the same format. But could you follow one of the current ones?
https://github.com/catcherwong/Nancy.Swagger/blob/ae38a293c225ff29d3bf6ada755847316c705e69/test/Nancy.Swagger.Tests/SwaggerModelDataBuilderTests.cs
The naming bothers me with the underscores.
There was a problem hiding this comment.
Oh really? I'm actually usually a fan of using _ to separate sentences like that:
[Should|ShouldNot][DoThisThing][When|If|ThisThing]
There was a problem hiding this comment.
I just feel like the keywords are helpful enough.
I'm fine either way though - I mostly brought it up because there was the precedent not to use them. Maybe rename the existing functions then?
There was a problem hiding this comment.
I find some Open Source projectss use this style for their tests , such as NancyFx . I use _ to separate for both of my own projects as well , so what I did is just follow this style .
|
This is a good start for tests though! I'll approve once the function names are changed. Don't worry about the build issue - I'll make a new issue for that. |
src/Swagger.ObjectModel/Utilities.cs
Outdated
| @@ -5,9 +5,14 @@ namespace Swagger.ObjectModel | |||
| { | |||
| public static class Utilities | |||
| { | |||
| /// <summary> | |||
| /// convert a string to camelcase with special rules. | |||
| /// </summary> | |||
There was a problem hiding this comment.
Not a fan of redundant comments usually
There was a problem hiding this comment.
All right , this comment needs to be remove !
| [Fact] | ||
| public void Should_AbleToSetNameAndSchema() | ||
| { | ||
| // Arrange |
There was a problem hiding this comment.
Unnecessary comments
EDIT: I see now that you probably added these comments to be more in line with the tests that already exist. Don't follow that pattern, I'll clean those up
| private readonly BodyParameterBuilder builder; | ||
| private readonly string name; | ||
| private readonly Schema schema; | ||
| public BodyParameterBuilderTest() |
|
These look pretty good to me aside from the very minor things I've pointed out. Thanks for contributing! |
|
Holding off on merging until jnallard and I sync up on the test name thing |
|
@yahehe I think Swagger.ObjectModel is more easier to begin the tasks and consider somethings need to be unified . so I choose it for the beginning ! |
|
Spoke to @yahehe and we agreed that you could use the underscores for the test names. |
|
@jnallard OK , I will make revisions accordingly which are based on your reviews ! Thank you ! |
After reading the #127 , I use my spare time to begin this task .
But based on existing test code , I can not clearly know the code style this project wants to have !
So I choose Swagger.ObjectModel as my first step and do some tests .
And these tests need you review and suggestion whether the test code fit this project.
For this PR , what I have done are as following