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

Report binding problems to binding registry and show them as errors during test execution #2663

Merged
merged 6 commits into from
Nov 9, 2022

Conversation

gasparnagy
Copy link
Contributor

@gasparnagy gasparnagy commented Oct 24, 2022

Some generic binding errors (e.g. [Binding] on a generic type) were not properly reported as errors so far, only as warnings, but those are usually not visible by test runners. Some of these errors were causing strange exceptions during execution, but it was hard to trace them back to the source problem.

Also type load exceptions blocked the discovery process that causes problems with tooling (e.g. VS) that uses the binding registry builder to discover the step definitions.

This PR introduces validity of loaded binding registry and provides a way to query these errors from the binding registry. During test execution, the invalid binding registry fails the tests by listing all collected error.

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).
  • Performance improvement
  • Refactoring (so no functional change)
  • Other (docs, build config, etc)

Checklist:

  • I've added tests for my code. (most of the time mandatory)
  • I have added an entry to the changelog. (mandatory)
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@gasparnagy gasparnagy force-pushed the report_binding_problems_as_errors branch from 44f34f9 to 5419212 Compare October 25, 2022 18:20
@gasparnagy gasparnagy marked this pull request as ready for review October 25, 2022 18:27
@gasparnagy gasparnagy changed the title Report binding problems as errors Report binding problems to binding registry and show them as errors during test execution Oct 25, 2022
@gasparnagy
Copy link
Contributor Author

@SabotageAndi the PR exposes a tuple on the IBindingRegistry interface (IEnumerable<(BindingErrorType ErrorType, string Message)> GetErrorMessages();) I don't know if this is a good practice, or whether we should create a new type for that. What do you think?

@SabotageAndi
Copy link
Contributor

I would not put a tuple into a public interface. Feels wrong for me.

@gasparnagy
Copy link
Contributor Author

@SabotageAndi OK, I agree. I will fix it

@gasparnagy
Copy link
Contributor Author

@SabotageAndi pushed a new version without the tuple

@SabotageAndi
Copy link
Contributor

@gasparnagy Ready to merge?

@gasparnagy gasparnagy merged commit 697fe33 into master Nov 9, 2022
@gasparnagy gasparnagy deleted the report_binding_problems_as_errors branch November 9, 2022 06:38
@gasparnagy
Copy link
Contributor Author

@SabotageAndi yes, done.

gasparnagy added a commit that referenced this pull request Nov 9, 2022
* master:
  Report binding problems to binding registry and show them as errors during test execution (#2663)
  Fix for SF2649: Aggregate exceptions lost from async step definitions (#2667)

# Conflicts:
#	TechTalk.SpecFlow/Bindings/BindingInvoker.cs
#	Tests/TechTalk.SpecFlow.RuntimeTests/Bindings/BindingInvokerTests.cs
Code-Grump pushed a commit to Code-Grump/SpecFlow that referenced this pull request Mar 29, 2023
…uring test execution (SpecFlowOSS#2663)

* Refactor BindingSourceProcessor to collect binding errors

* Fix BindingSourceProcessorStub

* Add tests & fixes

* collect type load errors

* Extend changelog

* Replace tuple with a class on the IBindingRegistry interface
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