Add unit tests for better coverage in Execution namespace

[lg2de]

I've created some new tests (and minor refactoring) for better coverage in the namespace Execution according to #1823.

[coveralls]

Pull Request Test Coverage Report for Build 3641237862

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 14 of 14 (100.0%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.1%) to 97.031%

---

Totals
Change from base Build 3568109486:	0.1%
Covered Lines:	12463
Relevant Lines:	12692

---

💛 - Coveralls

[jnyrup]

I'm wondering if we can test this through the public API instead of accessing internals.

E.g. could we add a test project with an custom/unsupported test framework to exercise when !framework.IsAvailable?

[lg2de]

I'm wondering if we can test this through the public API instead of accessing internals.

Currently the analysis of test framework is not public. So it cannot be tested with public API.
Should we change this?

E.g. could we add a test project with an custom/unsupported test framework to exercise when !framework.IsAvailable?

I guess this is also currently not possible because the API for "custom test framework" is not possible.

[jnyrup]

Currently the analysis of test framework is not public. So it cannot be tested with public API.
Should we change this?

By public API I intended to mean through some part of the existing public API that can reach TestFrameworkProvider, which seems (I'm not at my development) to be Services.Throw.

I can recommend reading Unit testing internals for some interesting thoughts on this subject.

[lg2de]

Within really existing testing frameworks (which are used in the FA solution) the FallbackFramework is never instantiated.
It is not possible to change the configuration to use the fallback or any dummy because Services.Configuration is getter only.
Finally FallbackFramework is internal.
So, how to test it without InternalsVisibleTo?
It would require "major" refactoring to test these classes.

[dennisdoomen]

E.g. could we add a test project with an custom/unsupported test framework to exercise when

That's exactly what we should be doing instead.

[lg2de]

What is you preferred unsupported framework?

[dennisdoomen]

What is you preferred unsupported framework?

Good point. I think the best approach is to allow overriding the test framework provider through AssertionOptions and add that test to the correponding specs.

[lg2de]

Mmh, AssertionOptions is a static class which cannot savely be changed for a single test only.
I guess it will get quite complicated to get progress here.

[lg2de]

I've removed the tests which requires (additional) InternalsVisibleTo.
Refactoring AssertionOption needs more discussion. I guess this could be a target for v7.

[dennisdoomen]

🤔 I have to admit that I don't like this approach where we make private stuff internal. Instead, I prefer that we make the list of test frameworks configurable. In other words, we allow people to modify the Frameworks collection through AssertionOptions (in the AssertionOptionSpecs. Then you just empty the collection and observe the effect.

[lg2de]

🤔 I have to admit that I don't like this approach where we make private stuff internal. Instead, I prefer that we make the list of test frameworks configurable. In other words, we allow people to modify the Frameworks collection through AssertionOptions (in the AssertionOptionSpecs. Then you just empty the collection and observe the effect.

As I already wrote above, making used testing framework configurable is very difficult, because AssertionOptions is a static class. Multiple tests running in parallel cannot configure different testing frameworks. I guess this requires major rework in V7. (I'll make a separate issue therefore.)

The current version of the PR does not change the public API. So, what is the problem with the proposed changes?

[dennisdoomen]

As I already wrote above, making used testing framework configurable is very difficult, because AssertionOptions is a static class. Multiple tests running in parallel cannot configure different testing frameworks. I guess this requires major rework in V7. (I'll make a separate issue therefore.)

The AssertionOptionsSpecs don't run in parallel with any other test and are a safe place to add these kinds of tests.

[lg2de]

The mentioned tests in the context of AssertionOptions I'll take care separately.

Hope the PR is acceptable now.

[dennisdoomen]

Instead of making certain members internal, I thought we agreed to do the following:

• Make the test framework configurable through the AssertionOptions
• Move the tests to the AssertionOptionsSpecs?

[lg2de]

I made internal member of an internal class. Is this that bad?
Configuration using AssertionScope is required for the tests I already removed from this PR.

[dennisdoomen]

I made internal member of an internal class. Is this that bad?

You're testing the internals of a class. That's something we should avoid at all costs. If we would change the internals, it should not break existing tests. That's my golden rule of thumb...

[jnyrup]

Make the test framework configurable through the AssertionScope

You still mean AssertionOptions, right?
AssertionScopes are constructed over and over, but you don't want/can't to change test framework provider in the middle of a test run.

Currently if a user wants to provide their own throwing mechanism, they can assign one to Services.ThrowException

[dennisdoomen]

Make the test framework configurable through the AssertionScope

You still mean AssertionOptions, right? AssertionScopes are constructed over and over, but you don't want/can't to change test framework provider in the middle of a test run.

Currently if a user wants to provide their own throwing mechanism, they can assign one to Services.ThrowException

Yes, I meant AssertionOptions.... 🤦‍♂️

[lg2de]

Now I have removed the last commit too.
The effort to create code coverage on TestFrameworkProvider without accessing internals is IMHO too high.

[jnyrup]

@dennisdoomen How about this approach to make TestFrameworkProvider testable without exposing private methods or changing the public API.

jnyrup@eea1786

[dennisdoomen]

@dennisdoomen How about this approach to make TestFrameworkProvider testable without exposing private methods or changing the public API.

jnyrup@eea1786

Love it!

[jnyrup]

@lg2de do you want to incorporate the changes from my commit above?
I don't want to steal your hard work.