Clean-up and finalization of data set support - #3
Conversation
9ffc836 to
eca2d43
Compare
06f85d0 to
d365137
Compare
d365137 to
b86715b
Compare
|
@jnyrup completely forgot about the PR. Thanks for the keen eye. |
jnyrup
left a comment
There was a problem hiding this comment.
This is my final review on the essentials, i.e. the public parts.
| @@ -59,25 +51,9 @@ | |||
| <PackagePath>contentFiles\any\any\</PackagePath> | |||
There was a problem hiding this comment.
Is this needed?
<ItemGroup>
<None Include="InitializeDataSetSupport.cs" >
<Pack>true</Pack>
<PackagePath>contentFiles\any\any\</PackagePath>
</None>
</ItemGroup>There was a problem hiding this comment.
Yes, because this .cs is added to the project that uses this package and will allow this package to add the specific implementations of IEquivalencyStep needed for working with DataSet.
There was a problem hiding this comment.
Isn't that done in FA core via assembly scanning?
I've tried removing this inclusion of InitializeDataSetSupport.cs in the nuget package, creating a nuget package of this project and then referencing that nuget package from a separate solution.
From that other solution I could successfully run When_data_sets_are_identical_equivalence_test_should_succeed.
There was a problem hiding this comment.
- This package will inject
InitializeDataSetSupport.csto the test project that adds a[AssertionEngineInitializer]to the test project that ensuresDataSetsupport is configured correctly. - Before the first assertion happens, the new assembly scanning feature introduced in v7 searches for the
[AssertionEngineInitializer]and causes the
Without this, it all depends on when the FluentAssertions.DataSets package is loaded. With all the lazy-loading .NET is doing, this results in a very unpredictable behavior.
There was a problem hiding this comment.
Roger 👍
So because GetAssemblies only loads at currently loaded assemblies, we might miss FluentAssertions.DataSet.
https://github.com/fluentassertions/fluentassertions/blob/aff3f7829056083c5280f7c0f530df5645d83dc0/Src/FluentAssertions/Common/Services.cs#L83-L88
Then how about adding <Visible>false</Visible> to hide it?
In cases of transitive inclusion, e.g. TestProject -> MiddleProject -> FluentAssertions.DataSet, do you know if this adds InitializeDataSetSupport to TestProject and/or MiddleProject?
There was a problem hiding this comment.
So because
GetAssembliesonly loads at currently loaded assemblies, we might miss FluentAssertions.DataSet.
Exactly. And GetReferences only gets the assemblies that are needed by the code currently loaded.
Then how about adding false to hide it?
What do you mean? To hide it from the solution explorer when opening FluentAssertions.DataSets
In cases of transitive inclusion, e.g. TestProject -> MiddleProject -> FluentAssertions.DataSet, do you know if this adds InitializeDataSetSupport to TestProject and/or MiddleProject?
It'll be added to MiddleProject
What does that mean? You don't want to review it again? |
Updated the package references in the csproj files. Refactored the code in several files to improve readability and maintainability. Also, fixed the namespace in the DataTableAssertionExtensions and DataRowAssertionExtensions classes to align with standard conventions. No functional changes were made.
b86715b to
9e7bad2
Compare
fluentassertions/fluentassertions#2002
IMPORTANT