-
Notifications
You must be signed in to change notification settings - Fork 716
Add support for asserting on DataSet, DataTable, DataColumn, DataRow #1419
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Impressive work. I didn't even know people are still using data sets.
❌ Please properly document all types and all public, protected members, especially everything that is exposed to consumers of this library.
🔧 I suggest to rebase this PR on top of #1379. It will be merged soon and contains some breaking changes.
🔧 Most of the review comments apply on many places on this PR. I only mention an issue once, and hope you'll fix similar cases everywhere.
Src/FluentAssertions/Equivalency/ConstraintCollectionEquivalencyStep.cs
Outdated
Show resolved
Hide resolved
Src/FluentAssertions/Equivalency/ConstraintCollectionEquivalencyStep.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, wasn't finished with the review...
❌ Please also update the release notes and documentation as mentioned in the PR template.
Src/FluentAssertions/Equivalency/DataRelationEquivalencyStep.cs
Outdated
Show resolved
Hide resolved
Src/FluentAssertions/Equivalency/DataRelationEquivalencyStep.cs
Outdated
Show resolved
Hide resolved
Tests/FluentAssertions.Specs/Equivalency/DataEquivalencySpecs.cs
Outdated
Show resolved
Hide resolved
85850f9
to
3d7e0a2
Compare
I found a bug in #1379 while trying to rebase onto it. |
3d7e0a2
to
7c4269c
Compare
Rebased, all tests pass :-) If you foresee merging these changes in, just not before EOD today, would you consider labelling this PR with |
7c4269c
to
6d58ba9
Compare
I see #1379 merged, and have rebased onto |
You're still busy with this, right? |
I thought I was finished, did I miss something? docs/_pages/data.md |
Ohh, you mean XML docs in the code? I'll go fill them in. :-) |
475a1ba
to
cac5ac3
Compare
0f0ef37
to
185fb73
Compare
@dennisdoomen @jnyrup Can you give me any suggestions regarding performance? I'm trying to use this with
Is it super expensive to set up a nested equivalency context? |
I ran a profiler on it and came up with:
This last one looks like it is possibly by far the most expensive thing in the process. It seems to be triggered by So, I'm thinking:
and
Does that seem reasonable? Did I miss anything obvious? I'll try this later and see what happens. :-) |
I can confirm that the first change did indeed just about halve the running time. |
With |
Further improvement: By eliminating linear scans from |
A sample test run with 1,000,000 rows timed as follows:
|
With these performance changes, the performance of testing two large I'm not sure what other low-hanging opportunities there are for improvement. When I profile it now, the profiler tells me that 80% of the time is spent doing I/O apparently, but I can't seem to ever catch it doing I/O. Not sure what that's all about :-P |
I've added a profiling fixture for equivalency tests of large
This is a good performance characteristic, but the coefficient on it still feels high. I was hoping to set up a test in our codebase using Do you have any ideas off the top of your head about how to further improve the performance? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new commits looks good.
I had an extra look at the code and found a few more things.
I haven't reviewed the tests thoroughly, but just skimmed them and it looked good.
Src/FluentAssertions/Equivalency/DataRowCollectionEquivalencyStep.cs
Outdated
Show resolved
Hide resolved
Tests/FluentAssertions.Specs/Equivalency/DataEquivalencySpecs.DataColumn.cs
Outdated
Show resolved
Hide resolved
Tests/FluentAssertions.Specs/Equivalency/DataEquivalencySpecs.cs
Outdated
Show resolved
Hide resolved
Src/FluentAssertions/Equivalency/ConstraintCollectionEquivalencyStep.cs
Outdated
Show resolved
Hide resolved
Should I collapse these follow-up commits into the main commits, then? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just two small nits, then I'll be happy to merge this magnum opus into master.
Yes, please collapse the follow-up commits.
Not long ago I had to look at commit from 2012/2013 trying to understand some design decision.
It really helped that we had a clean git history.
Dennis wrote a blog post that elaborates more on the topic.
https://www.continuousimprover.com/2020/03/keep-source-control-history-clean.html
Tests/TestFrameworks/NSpec3.Net47.Specs/NSpec3.Net47.Specs.csproj
Outdated
Show resolved
Hide resolved
7e31403
to
39f7c6b
Compare
…ataTable, DataRow, and typed data tables). Preparatory work: - Refactored JoinUsingWritingStyle out of Formatting/TimeSpanValueFormatter.cs into an extension method in new file Formatting/EnumerableExtensions.cs. - Added unit tests of JoinUsingWritingStyle in EnumerableExtensionSpecs.cs. - Added a performance optimization to GenericDictionaryEquivalencyStep.cs and GenericEnumerableEquivalencyStep.cs to avoid having TypeExtensions.GetClosedGenericInterfaces call .GetInterfaces() if the TypeCode indicates that the value couldn't possibly implement the specified interface to begin with. Added equivalency steps for dealing with DataSet-related types: - DataSetEquivalencyStep.cs - DataRelationEquivalencyStep.cs - DataTableEquivalencyStep.cs - DataColumnEquivalencyStep.cs - ConstraintCollectionEquivalencyStep.cs - ConstraintEquivalencyStep.cs - DataRowCollectionEquivalencyStep.cs - DataRowEquivalencyStep.cs Updated references to system assemblies as needed: - Added reference to System.Data and System.Data.DataSetExtensions from FluentAssertions.csproj for the net47 target framework. - Added NuGet references to System.Data.DataSetExtensions for the netcoreapp2.1 and netstandard2.0 targets in FluentAssertions.csproj. - Added a package reference to System.Data.DataSetExtensions from Approval.Tests.csproj. - Updated Benchmarks.csproj and NSpec3.Net47.Specs.csproj to reference System.Data.DataSetExtensions in the target framework. Registered these new IEquivalencyStep implementations in GetDefaultSteps in EquivalencyStepCollection.cs. Added new type DataEquivalencyAssertionOptions<T>. Added a new overload of CloneDefaults to AssertionOptions.cs to support cloning to specific types. Added new subclasses of ReferenceTypeAssertions<TSubject, TAssertions> to support tailored Should() options for DataSet and related types: - DataRowAssertions.cs - DataTableAssertions.cs - DataColumnAssertions.cs - DataRowAssertions.cs Added supporting type RowMatchMode.cs. Added a new overload of .Should() to AssertionExtensions.cs for DataColumn, and added DataSetAssertionExtensions.cs, DataTableAssertionExtensions.cs and DataRowAssertionExtensions.cs to provide Should<T> overloads specializing on DataSet, DataTable and DataRow (allowing the compile-time type to be captured). Updated TypeExtensions.cs to short-circuit GetClosedGenericInterfaces if the type isn't an object type. Added unit tests of all the different paths through the code in the new equivalency steps to FluentAssertions.Specs.csproj as Equivalency/DataEquivalencySpecs.*.cs. Added a reference to System.Data.DataSetExtensions in order to use TypedTableBase<T>. Ran AcceptApiChanges.ps1.
- Added LargeDataTableEquivalency.cs to the Benchmarks project with code to predictably populate two identical DataTables then measure the time taken to ensure that they are equivalent. - Added benchmark class UsersOfGetClosedGenericInterfaces.cs to the Benchmarks project. - Added references to the Bogus library and to System.Collections from Benchmarks.csproj. - Updated Program.cs to run the LargeDataTableEquivalency benchmark.
- Added new file docs/_pages/data.md with documentation of System.Data assertions. - Updated docs/_data/navigation.yml to link the System.Data documentation into the navigation tree. - Added mention of the System.Data functionality in the Release Notes (docs/_pages/releases.md).
|
Don't worry about that. We don't care ;-) |
@logiclrd Thanks for taking the time to implement this feature and having the patience to listen to our concerns during the review process. |
Awesome work @logiclrd 👌👌 |
Thanks so much, it's exciting to be part of such a widely-used library :-) Your detailed reviews definitely made a big difference to the quality. |
I tried asserting equivalency on
DataSet
objects and the results were .. dissatisfying. :-)This PR:
DataSet
and related types.GetDefaultSteps
inEquivalencyStepCollection.cs
..Should()
inAssertionExtensions.cs
drive by new subclasses ofReferenceTypeAssertions<>
.IMPORTANT
AcceptApiChanges.ps1
/AcceptApiChanges.sh
.