8000 Add unit tests for better coverage in Execution namespace by lg2de · Pull Request #2042 · fluentassertions/fluentassertions · GitHub
[go: up one dir, main page]
Skip to content

Add unit tests for better coverage in Execution namespace#2042

Merged
dennisdoomen merged 5 commits intofluentassertions:developfrom
lg2de:tests-for-execution-coverage
Jan 21, 2023
Merged

Add unit tests for better coverage in Execution namespace#2042
dennisdoomen merged 5 commits intofluentassertions:developfrom
lg2de:tests-for-execution-coverage

Conversation

@lg2de
Copy link
Contributor
@lg2de lg2de commented Nov 22, 2022

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

@lg2de lg2de force-pushed the tests-for-execution-coverage branch from 563259f to faadcc6 Compare November 23, 2022 09:47
@coveralls
Copy link
coveralls commented Nov 23, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 3924366754

Details

  • 6 of 12 (50.0%) changed or added relevant lines in 3 files are covered.
  • 10 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+1.0%) to 96.955%
Changes Missing Coverage Covered Lines Changed/Added Lines %
Src/FluentAssertions/Common/Configuration.cs 1 2 50.0%
Src/FluentAssertions/Execution/TestFrameworkProvider.cs 4 9 44.44%
Files with Coverage Reduction New Missed Lines %
Src/FluentAssertions/Common/Configuration.cs 4 81.25%
Src/FluentAssertions/Execution/TestFrameworkProvider.cs 6 53.19%
Totals Coverage Status
Change from base Build 3923949427: 1.0%
Covered Lines: 12476
Relevant Lines: 12715
💛 - Coveralls

@jnyrup
Copy link
Member
jnyrup commented Dec 2, 2022

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
Copy link
Contributor Author
lg2de commented Dec 2, 2022

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
Copy link
Member
jnyrup commented Dec 2, 2022

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
Copy link
Contributor Author
lg2de commented Dec 2, 2022

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
Copy link
Member
dennisdoomen commented Dec 3, 2022

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
Copy link
Contributor Author
lg2de commented Dec 3, 2022

What is you preferred unsupported framework?

@dennisdoomen
Copy link
Member
dennisdoomen commented Dec 4, 2022

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
Copy link
Contributor Author
lg2de commented Dec 5, 2022

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 lg2de marked this pull request as draft December 5, 2022 19:58
@lg2de lg2de force-pushed the tests-for-execution-coverage branch from 7e19645 to cfba009 Compare December 7, 2022 17:19
@lg2de
Copy link
Contributor Author
lg2de commented Dec 7, 2022

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

@lg2de lg2de marked this pull request as ready for review December 7, 2022 17:26
@jnyrup jnyrup added the testing Testing of Fluent Assertion itself label Dec 17, 2022
jnyrup
jnyrup reviewed Dec 19, 2022
View reviewed changes
dennisdoomen
dennisdoomen reviewed Dec 25, 2022
View reviewed changes
Copy link
Member
@dennisdoomen dennisdoomen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 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
Copy link
Contributor Author
lg2de commented Dec 28, 2022

🤔 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
Copy link
Member
dennisdoomen commented Dec 28, 2022

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 lg2de force-pushed the tests-for-execution-coverage branch 2 times, most recently from 27a5d98 to 6cf834b Compare January 14, 2023 18:09
@lg2de lg2de force-pushed the tests-for-execution-coverage branch from 6cf834b to f9a958a Compare January 14, 2023 18:12
@lg2de
Copy link
Contributor Author
lg2de commented Jan 14, 2023

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

Hope the PR is acceptable now.

jnyrup
jnyrup approved these changes Jan 14, 2023
View reviewed changes
@dennisdoomen
Copy link
Member
dennisdoomen commented Jan 15, 2023 •
edited
Loading

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
Copy link
Contributor Author
lg2de commented Jan 15, 2023

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.

@lg2de lg2de force-pushed the tests-for-execution-coverage branch from f9a958a to 995bdc7 Compare January 15, 2023 14:06
This was referenced Sep 15, 2025
Closed
Closed
This was referenced Sep 25, 2025
Closed
Closed
Closed
Closed
This was referenced Oct 3, 2025
Closed
Closed
@dependabot dependabot bot mentioned this pull request Oct 13, 2025
Closed
This was referenced Oct 20, 2025
Closed
Closed
Open
Open
This was referenced Oct 27, 2025
Closed
Closed
This was referenced Nov 10, 2025
Closed
Closed
This was referenced Nov 24, 2025
Closed
Open
This was referenced Dec 1, 2025
Closed
Closed
Merged
Closed
This was referenced Dec 22, 2025
Open
Closed
This was referenced Jan 11, 2026
Merged
Open
@dependabot dependabot bot mentioned this pull request Feb 8, 2026
Merged
This was referenced Mar 2, 2026
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dennisdoomen dennisdoomen dennisdoomen approved these changes

@jnyrup jnyrup jnyrup approved these changes

Assignees

No one assigned

Labels

testing Testing of Fluent Assertion itself

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@lg2de @coveralls @jnyrup @dennisdoomen
0