-
Notifications
You must be signed in to change notification settings - Fork 84
Added ReportModel for reports #202
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there an issue with the 2.0 runner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, should I put it back to xunit 2.0?
If we are introducing a reporting model, it would be good to raise the Examples property up a level so it's on the Story not the scenario. That would remove the need for story.Scenarios.First().Example? |
Changed ReflectiveWithExamples test to have an AndGiven step which produced the additional redundant AndThen step. Modified MethodNameStepScanner to return the first match it finds. Not sure why it would return multiple steps for one method (unless it is to do with RunStepWithArgs)? No tests failed so I'm hoping that's not the case.
@MehdiK thoughts on the PR. I don't know the reporting stuff that well? @mwhelan it would be good to separate the fix for #212 out of this PR so they can be reviewed/merged separately. To work some git magic and sort all this out:
Then close this PR and open two more from the two different branches :) I can do this for you btw, but just incase you want to learn some gitfoo. |
Thanks @JakeGinnivan . Yeah, if you could do it that would be awesome. My head is full of Service Fabric today and no room for gitfoo! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid confusion, do you think it's worth calling these StoryModel, ScenarioModel etc?
Thanks for the PR @mwhelan. I am going to sound like a jerk and I apologise. I remember we came to the conclusion that this was a good idea; but I don't remember the reasons. Would you mind refreshing me please? I am just concerned about name reuse confusion as well as the effort required to keep these two models in sync. Is it possible to avoid this change by exposing the bits that should be serialised as public getters? |
At work we built something to persist reports from each test assembly into RavenDb and then generated an aggregated report from RavenDb. There were issues with serializing the domain objects in RavenDb (it was a few months ago and the exact errors escape me). When we discussed it at the time, we liked the idea of a simple POCO report model, for the same reasons you would create a view model in MVC. The report model can vary independently of the domain model (such as @JakeGinnivan suggestion above to raise the Examples property up a level to the Story) and it avoids having to make changes to the domain model just to suit reporting needs. I was originally going to create independent model classes, such as StoryModel/ScenarioModel etc., but during implementation preferred the self-contained report model, for the reasons stated above. I think the name reuse (nested in the report model) actually avoids confusion. I don't see an issue with keeping the models in sync (the tests for the mapping cover that). And the goal is not necessarily to keep them in sync but to allow the report model to vary independently of the domain model. I also think the report model makes it simpler for new users to add a new report as it's much easier to engage with that one class than have to dig through the whole code base. |
Thanks for explaining @mwhelan. It all make sense now. |
@mwhelan if you persist this model directly into raven there may be issues deserialising again if we make breaking changes. |
Rebased and merged manually @ ea6040d |
Discussed this with Mehdi. Was persisting BDDfy reports to Raven and there were issues with serializing/deserializing the domain objects used in the reports. This PR maps the domain objects to a ReportModel of POCOs just for reports.