Skip to content
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

Implement failure reporting #91

Merged
merged 4 commits into from
Nov 25, 2020
Merged

Conversation

gnunicorn
Copy link
Contributor

Currently when running Cucumber..run() nothing it reported back to the caller. This changes this behavior by making the result value a result with either () in the regular case (arguably could be the number of successes or something) and u64 if any errors have been found. This number also equals the total number of failure found.

To implement this a handy function failed() is added to the various events allowing for checking down the tree whether the currently reported item reports a failure or not. All functions have minimal rustdoc documentation added.

@gnunicorn
Copy link
Contributor Author

Sorry, only saw now that there is another PR pending implementing result reporting on run. However, it was moved into draft without further notice. Is this generally unwanted? What is the approach thought to be used instead?

@bbqsrc
Copy link
Member

bbqsrc commented Nov 24, 2020

Thanks for the contribution! The preferred approach has been discussed here: #84 (comment).

I think with a little rework your PR can solve this issue. 😄

@gnunicorn
Copy link
Contributor Author

gnunicorn commented Nov 24, 2020

yeah, that's easy enough to implement.

So I guess this means we want it to report that new Output-type in any case (rather than a Result) and have that inform us whether failures where found?

Only remaining question then would be whether to implement that reporting on the EventHandler or raw in the Cucumber-Command. I'd argue the latter as it is generic over different handlers, but that also means the current "tracking" would happen twice or we'd have to alter the EventHandler to take the final counts from Cucumber and not count itself internally... not a clean design either way. Thoughts?

@bbqsrc
Copy link
Member

bbqsrc commented Nov 24, 2020

Not duplicating the count is the best option. Other APIs can be refactored later. Ideally counting things would be functionally separate to the event handlers, as they exist to enable pretty output, XUnit and other 'outputters', and should not have to care about reimplementing the counting logic, as it must be the same anyway.

Perhaps some kind of counter type can be passed around with an Arc to deduplicate the counts in the first place.

@gnunicorn
Copy link
Contributor Author

@bbqsrc what do you think about this way?

src/cucumber.rs Outdated Show resolved Hide resolved
src/cucumber.rs Outdated Show resolved Hide resolved
@bbqsrc
Copy link
Member

bbqsrc commented Nov 24, 2020

Can you rebase onto the current main and force push your branch? CI should hopefully run then. 🙂

@bbqsrc
Copy link
Member

bbqsrc commented Nov 24, 2020

Nice. 😄 Any other changes or do you think it's ready?

@gnunicorn
Copy link
Contributor Author

Should be good to go :) .

@bbqsrc bbqsrc merged commit ad6c23c into cucumber-rs:main Nov 25, 2020
@bbqsrc
Copy link
Member

bbqsrc commented Nov 25, 2020

Thanks! Only thing left from my original suggestion is a run_and_exit function, so I would happily accept a PR for that, otherwise I'm sure someone will get to it, if not me. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants