Add a few more tests for JsonWriter and YamlWriter#40
Conversation
PerthCharern
commented
Oct 26, 2017
PerthCharern
commented
- Add a few more tests for JsonWriter and YamlWriter
- The YamlWriter itself still has quite a few bugs. I'm working on that and will send out a separate PR.
|
|
||
| public OpenApiJsonWriterTests(ITestOutputHelper output) | ||
| { | ||
| _output = output; |
There was a problem hiding this comment.
remember to remove the console output #Resolved
There was a problem hiding this comment.
I'm always conflicted with this. Every time you need to debug a unit test, it's impossible to do without console output. But then people don't want console output in the test because it's not clean :(
I think we should leave the constructor here at least, so that when someone needs to debug something, they don't need to start from scratch. I'll remove the calls _output.WriteLine().
|
|
||
| [Theory] | ||
| [MemberData(nameof(WriteListData))] | ||
| public void WriteList(string[] stringValues) |
There was a problem hiding this comment.
make the test case function name more meaningful? #Resolved
|
|
||
| using System; | ||
|
|
||
| namespace Microsoft.OpenApi |
There was a problem hiding this comment.
why do you think to push such extension in the Core lib? #Resolved
There was a problem hiding this comment.
I was thinking this could be useful for serialization. But maybe I was wrong, I'll put it in the Unit Tests for now.
| } | ||
|
|
||
| protected void ValifyCanWritePropertyName(string name) | ||
| protected void VerifyCanWritePropertyName(string name) |
| </ItemGroup> | ||
| <ItemGroup> | ||
| <Compile Include="ExampleTests.cs" /> | ||
| <Compile Include="OpenApiJsonWriterTests.cs" /> |
There was a problem hiding this comment.
Put the test C# files into the corresponding sub folders?
- Move string extensions to unit test project - remove console output from unit test
|
@xuzhg I addressed your comments above. Feel free to let me know if there's anything else. |
|
Others look good to me. |
|
@PerthCharern please use "Rebase and Merge" button to merge. Then we can have the straight line history. |
|
@xuzhg I'm not a big fan of rebase and merge. The small commits are not complete on its own and it's difficult to figure out "one change" once we do rebase and merge. I can do it for this one but we should have a discussion on the policy on which kind of merge is allowed at some point. This is lower priority though, so we can do it whenever we have down time. |
|
@xuzhg Actually, if you don't mind, I won't do rebase and merge for this PR, since github is complaining that there are conflicts when I do rebase and merge, but NOT when I do normal merge. I don't want to spend time on git technology at this point. There's a potential we might have to clean up history before release anyway (I think...). |
|
@PerthCharern Got it. |
|
@PerthCharern It seems you removed a I don't think we should always add a "new line" for the top level property. Right? |
|
YamlWriter has a lot of issues. I need to look at the PR to recall the exact reason, but that check was causing some inconsistencies. I'll get back to you. |
|
@xuzhg Technically, YAML can have --- to indicate the starting point of the document. This means that you'll need to have the first newline before the first property as I see in most docs here: http://www.yaml.org/spec/1.2/spec.html Even without ---, I don't think adding a newline is wrong. Always adding newline for property seems more consistent to me in that regard. Unless of course, you think this violates the YAML spec. |
|
No, I don't mean it's wrong or violate the spec. @darrelmiller what's your opinion? |
|
@xuzhg Yeah, I understand your point. I don't mind either way. The aesthetics depends on whether we want to have --- at the beginning of the YAML doc. |
|
Let me file an issue to track on this issue. |
|
Oh, we have it: #39. Let's use it. |
2 participants
