Add testing documentation #1416
Conversation
|
@microsoft-github-policy-service agree |
|
Hello @csc530, thank you for this contribution, enhancing documentation is always welcome. I'd like to make a few grammar/stylistic changes (to align with the rest of the documentation). Are you happy for me to force-push to this branch? |
|
Yeah, that would be fine Also, I wasn't sure how to add the new testing documentation section to the sidebar, I tried adding |
I can't exactly remember but will take a look. |
|
This is now top of my TODO stack to get over this line. |
| return result; | ||
| } | ||
|
|
||
| public static void appConfig(IConfigurator config) |
There was a problem hiding this comment.
Can we rename this to something like ConfigureCommandApp?
| { | ||
| public override int Execute(CommandContext context) | ||
| { | ||
| AnsiConsole.WriteLine("Hello world"); |
There was a problem hiding this comment.
Command should not use AnsiConsole directly, since this doesn't play well with testing. The command should inject a IAnsiConsole and use that.
| { | ||
| // Given | ||
| var console = new TestConsole(); | ||
| // When |
| var console = new TestConsole(); | ||
| // When | ||
| console.WriteLine("Hello world"); | ||
| // Then |
| public class ProgramTests | ||
| { | ||
| [Fact] | ||
| public void Should_Return_Hello_World() |
|
Dear @csc530, thank you for taking the time (and initiative) to submit this PR. I've taken the time to review it, but also more importantly, to step back and review the bigger need for testing documentation. Whilst I'm happy with your PR, the multiple various discussions else where about testing shows to me a much more comprehensive need to write up testing guidance. I'm sorry but on this occasion I'm going to close your PR, and if you like, you can follow my attempt to write something more detailed here: #1631 I am always looking for new contributors, however, so if you'd like to try your hand at another issue that would be most welcome. I suggest you tag me in before commencing any work, so I can provide guidance as you go along and discuss any issues etc, ahead of final review. It will ensure your contribution ends up getting merged. Once again, thank you, and please don't take this as any indication of your workmanship. |
|
Ok, thanks for the feedback :) |
|
Hello @csc530, sorry to bug you. Myself and the other maintainers have been discussing my closing of your PR. Basically, we've agreed that I should not have closed it, and should not have started authoring a replacement. We felt that behaviour might result in other contributors not stepping forward, and wasn't respectful of your time and effort. Instead, I should have spent more time with you and your PR, explaining what the bigger need was and helping you author that, if you were still interested. We could have just said nothing following our internal discussion, but I felt it only fair to write it here and admit my actions were not well considered (not usually the case for me). If I technically can, and if you agree, I'd like to add you as a co-author of my PR once finished, so you become part of the commit history of Spectre.Console, as a gesture of good wIll and to acknowledge your original PR. |
|
Oh!? Thank you, that would be really cool of you😊. I have looked around a little on the repo in discussions and issues about testing and documentation and gathered how my PR could be lacking. Honestly, this was more of a small starter PR to contribute to OSS and has been a good learning opportunity still. I've been busy too, so I'm not taking it too hard. I hope I can make time to follow your progress and find other ways to contribute to the project. Thx again |
|
Ok @csc530 , you are now the co-author of the following PR: https://github.com/spectreconsole/spectre.console/pull/1631/commits
We squash commits when merging, so the last message should disappear and it will be you and I as the authors of the commit to main. FYI. I maintain the command line subsystem of Spectre.Console, so anything with the https://github.com/spectreconsole/spectre.console/issues?q=is%3Aopen+is%3Aissue+label%3Aarea-CLI Just tag me directly in the issue before starting at any future point. Until then, be well. |
contributes to #1410
Changes
I added pages three new pages to the docs related to the testing namespace of Spectre Console.
It gives examples and some usage and use cases for the testing classes (that are hopefully correct and reflect reality).
The documentation is written from what I could find, understand, and have done using Spectre Console.
Please upvote 👍 this pull request if you are interested in it.