Basic comments handling #311
Conversation
|
Cool, I like it. I had a quick review of the code and I think the overall direction is good. So please go ahead and add handling for the edge cases you mentioned. One concern - it's a bit of a mystery to me: the continuous build successfully built your pull request, but when I checked it out locally and ran the tests, two of them were failing. Both of them in the FormattingAFeature file. The reason is that the expected JSON did not include arrays for the comments. Could you check that in your version please! And again, thanks for this big contribution. It would be cool if you could finalize it this week so it can be released in time for the CukeUP conference next week :-) |
|
Thanks for the feedback ! it's doable this week, I'll add the missing commits. Concerning the two tests, you are right it is missing the comment arrays on the step object, it will be added with the next commits. However these tests were already failing with the fork I did (still the head revision on develop). It looks like the "Summary" object is missing between "Features" and "Configuration". Is it correct ? |
|
I fixed the failing tests (by removing the "configuration" line which was an overspecification of the test). It's a bit of a mystery why the test sometimes passes and sometimes fails. I have my suspicions but no proof :-/ Anyway, you can now pull the latest commit from picklesdoc/develop with the fixed test. |
|
Hi, |
| // Set default | ||
| this.Type = CommentType.Normal; | ||
| } | ||
|
|
There was a problem hiding this comment.
This line contains whitespace, please remove it
| @@ -19,8 +19,11 @@ | |||
| // -------------------------------------------------------------------------------------------------------------------- | |||
|
|
|||
| using System; | |||
| using System.Collections.Generic; | |||
| using System.Linq; | |||
There was a problem hiding this comment.
Please remove this unused using
|
Thanks for your continuing efforts! I added a bunch of codestyle-related remarks to the files. I would also like you to write unit tests for the mapper, like the once you find in https://github.com/picklesdoc/pickles/tree/develop/src/Pickles/Pickles.Test/ObjectModel |
|
Hi, I've just finished the clean up + the test missing for the mapper, hopefully I didn't miss anything. |
|
So far so good - thanks! I also meant really granular tests where you test the mapping of Comment and Location in isolation, like for example the steps class mapping is being tested in https://github.com/picklesdoc/pickles/blob/develop/src/Pickles/Pickles.Test/ObjectModel/MapperTestsForStep.cs. |
+ more clean up
|
Ah alright, I've misunderstood the request, I've added two tests classes for comment and location, that should cover these cases |
|
Thanks for your time and energy! |
* Fix failing tests by removing overspecification * Untabify * Ensure description of feature is never null, fixes crashes of word and excel generation * Unit test * SpecFlow conformant name mapping * Implement name mapping in nunit signature builder more conformant to SpecFlows name generation logic wrt. to special characters * Unit tests for special character handling * Basic comments handling (#311) * basic comment handling * add excel handling * cleanup * clean up * fix tests * handle comments after the last step of a scenario * clean up * codestyle clean up * codestyle clean up * add test for mapper on comments * clean up * adding mapper tests on comments and location + more clean up * Update Nuget packages (and small fixes) (#310) * Update-Package SpecFlow * Update-Package Nunit * ExpectedException is obsolete in nunit3 * Update-Package System.IO.Abstractions.* * Refactor CurrentScenarioContext to be instance-based and to be injected * Update-Package MahApps.Metro * Update-Package AutoMapper * Update-Package Newtonsoft.Json * Remove IKVM assembly binding redirects * Do not use ScenarioContext.Current * tweak gitignore * Update GhostDoc file * Downgrade Automapper to last known good version * Exclude test project from test coverage * Edit applicationhost.config * Added support for multiple tags when searching (issue #283) (#319) * Use Gherkin 4 parser (#322) * Update-Package Gherkin * tweak applicationhost.config * Factory creates a GherkinDocument now * The parser now returns a GherkinDocument * Map a GherkinDocument to a Feature * Use new icon (#323) * Use new icon * Use a big png file with the new logo * Use the new .png file in the .nuspec files * Use visual studio 2015 * Version bump to 2.6.0 * Update change log
|
Released in version 2.6.0 |
Hello,
Related to issue #271, please find a basic handling of the comments at step level.
Based on the location (line, column) provided by the parser, the comments above the steps are considered commenting the step below. There is some edge cases not yet taken into account (comments below the last step, for instance); if you agree with the basic principle, I can make sure these are also taken into account.
There are also few tests checking the feature is working for the output formats.