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

Restore basic assertions for collections in System.Data

Merged
merged 36 commits into from Apr 16, 2022

Conversation

logiclrd
Copy link
Contributor

@logiclrd logiclrd commented Feb 16, 2022

In anticipation of the new functionality to allow excluding non-browsable fields (#1807), I updated our project to the latest published version and discovered that about 9 months ago, assertions on non-generic collections were removed from FluentAssertions. Unfortunately, DataSet and DataTable were codified before generics existed and have only non-generic collections. As such, assertions on these collections depended on the existing blanket non-generic collection functionality. With this removed, our test suite no longer compiles because of assertions such as,

result3.Rows.Should().BeEmpty();

This PR addresses this issue by wrapping the three collections involved in System.Data in a class that implements IEnumerable<T>, so that the assertions in GenericCollectionAssertions<T> can apply. In addition, some additional assertions are provided that take System.Data types as arguments.

This PR also includes updates to the release notes and documentation, as well as comprehensive automated testing of the new functionality.

IMPORTANT

  • The code complies with the Coding Guidelines for C#.
  • The changes are covered by unit tests which follow the Arrange-Act-Assert syntax and the naming conventions such as is used in these tests.
  • If the PR adds a feature or fixes a bug, please update the release notes with a functional description that explains what the change means to consumers of this library, which are published on the website.
  • If the PR changes the public API the changes needs to be included by running AcceptApiChanges.ps1 or AcceptApiChanges.sh.
  • If the PR affects the documentation, please include your changes in this pull request so the documentation will appear on the website.

@dennisdoomen
Copy link
Member

dennisdoomen commented Feb 17, 2022

Wow. Wasn't it easier to use Cast<T> instead?

@logiclrd
Copy link
Contributor Author

logiclrd commented Feb 17, 2022

We already have a suite of tests which I'd prefer to touch only if strictly necessary, and using Cast<T> doesn't have compile-time type safety.

@logiclrd
Copy link
Contributor Author

logiclrd commented Feb 17, 2022

The new assertions types and corresponding tests are very closely based on the existing collections assertion code; this is not created out of whole cloth, but out of copy/pasting the existing code and then massaging it to the types in question. :-)

@dennisdoomen
Copy link
Member

dennisdoomen commented Feb 17, 2022

Sorry, what I meant is that your new Should() methods would return the generic assertions by casting the non-generic collection to a generic-collection and then passing that to GenericCollectionAssertions.

@logiclrd
Copy link
Contributor Author

logiclrd commented Feb 17, 2022

Mm, that would provide part of the functionality. But, it wouldn't have things like ContainTableWithName and it would include a bunch of assertions that aren't relevant, such as NotBeNull.

@logiclrd
Copy link
Contributor Author

logiclrd commented Feb 17, 2022

Of course, there's no pre-existing code calling those methods. But given all the other System.Data-specific sets of assertions, this seemed the logical way to do it. Casting the collection and using GenericCollectionAssertions didn't occur to me, to be honest, but if it had, I think I still would have gone this route.

@dennisdoomen
Copy link
Member

dennisdoomen commented Feb 17, 2022

Mm, that would provide part of the functionality. But, it wouldn't have things like ContainTableWithName and it would include a bunch of assertions that aren't relevant, such as NotBeNull.

You could provide those as an extension method on GenericCollectionAssertions<DataSet> or something like that.

It's more to avoid too much duplication and maintenance on our side.

@logiclrd
Copy link
Contributor Author

logiclrd commented Feb 17, 2022

Hmm, that's true :-) I'll investigate that.

@logiclrd
Copy link
Contributor Author

logiclrd commented Feb 17, 2022

One thing with that approach is that after .Cast<DataTable>, the true subject's type is lost. The only way to check if the DataSet contains a table with the specified name, for instance, is to enumerate it; that's what .Contains will do on assertion.Subject, since it's an anonymous IEnumerable<DataTable> at that point wrapping the actual collection. DataTableCollection's own Contains method takes advantage of its internal data structure.

@dennisdoomen
Copy link
Member

dennisdoomen commented Feb 17, 2022

Is that a real issue? How many tables does a data set typically contain? I guess 50 is already quite a lot.

@jnyrup
Copy link
Member

jnyrup commented Feb 17, 2022

In it's current state I think this is duplicating too much functionality.
To be bluntly honest I still occasionally wonder whether #1419 should have been included in the core library.
Not to dishonest your work and contribution, but the truth is that the burden of support is left on @dennisdoomen and me.
E.g. #1068 (comment) revealed that #1419 seems to have a lot of untested cases.
Related blog post: https://blog.jessfraz.com/post/the-art-of-closing/

@logiclrd
Copy link
Contributor Author

logiclrd commented Feb 17, 2022

@dennisdoomen That's true... Is there no other situation where the loss of the subject type would be an issue?

@dennisdoomen
Copy link
Member

dennisdoomen commented Feb 17, 2022

To be bluntly honest I still occasionally wonder whether #1419 should have been included in the core library.

Have the exact same feeling. In retrospect, we should have decided to make it a supporting library that is maintained by others.

@dennisdoomen
Copy link
Member

dennisdoomen commented Feb 17, 2022

Is there no other situation where the loss of the subject type would be an issue?

Can't think of anything right now.

@logiclrd logiclrd force-pushed the JDG_DataSetCollections branch from 9f478c6 to f579d96 Compare Feb 17, 2022
@logiclrd
Copy link
Contributor Author

logiclrd commented Feb 17, 2022

I was hoping to get this in before the end of your day, but it was just too late for me. :-) Here's a rewrite that leverages GenericCollectionExtenions<TData>. I found that I still had to explicitly implement the equivalency checks for DataRow because what GenericCollectionExtensions does by default doesn't have the same semantics. But, there's a lot less code now!

@dennisdoomen
Copy link
Member

dennisdoomen commented Feb 18, 2022

While looking at the implementation and thinking of the repercussions of us maintaining this, I thought of another solution:

internal class EnumerableWrapper<T, TItem> : ICollection<TITem>, where T : ICollection

Then this wrapper delegates properties like Count to the wrapped part. Then instead of using Cast<TItem, you pass new EnumerableWrapper<DataTable, DataColumn>

Since Enumarable.Count() is smart enough to see that an IEnumerable<T> is really a collection, you don't get the penaalty.

@logiclrd
Copy link
Contributor Author

logiclrd commented Feb 18, 2022

That sounds promising! I'll give that a go.

@logiclrd
Copy link
Contributor Author

logiclrd commented Feb 18, 2022

Hmm, one problem: The non-generic ICollection is really sparse and doesn't have a Contains member. Could still be possible to have specialized implementations for the 3 collection types in question.

@logiclrd
Copy link
Contributor Author

logiclrd commented Feb 18, 2022

Huh, I think Visual Studio must have been acting up. IntelliSense was initially telling me that the non-generic interfaces didn't expose a Count method at all. So, I came up with the scheme in 68c839e that allows an abstract member to be implemented by collection-type-specific subclasses.

But, I kept poking at it because this didn't make sense to me and suddenly Count started showing up in IntelliSense. I'm confused, but I've reworked it to not need the collection-type-specific subclasses.

@logiclrd logiclrd force-pushed the JDG_DataSetCollections branch from 68c839e to a93a59b Compare Feb 18, 2022
@logiclrd
Copy link
Contributor Author

logiclrd commented Feb 18, 2022

There, I think that commit is what you had in mind, and it solves the problem elegantly. Thank you :-)

@dennisdoomen
Copy link
Member

dennisdoomen commented Feb 18, 2022

The non-generic ICollection is really sparse and doesn't have a Contains

What if you used InternalDataCollectionBase instead of ICollection?

@logiclrd
Copy link
Contributor Author

logiclrd commented Feb 18, 2022

What if you used InternalDataCollectionBase instead of ICollection?

The problem is more fundamental -- the Contains method overloads that I really care about are the ones that check for DataTable by TableName and DataColumn by ColumnName, and those can't be in common. DataRowCollection doesn't even have an analog for it at all. So, instead, I took advantage of the fact that the new wrapper class allows the underlying object to be inspected, and the assertions that want to call Contains simply call it directly on the underlying DataTableCollection or DataColumnCollection.

@dennisdoomen
Copy link
Member

dennisdoomen commented Feb 18, 2022

So, instead, I took advantage of the fact that the new wrapper class allows the underlying object to be inspected, and the assertions that want to call Contains simply call it directly on the underlying DataTableCollection or DataColumnCollection.

Fair enough. Let us know when you're ready for the formal review ;-)

@logiclrd
Copy link
Contributor Author

logiclrd commented Feb 18, 2022

I don't think I have anything outstanding on my end at this point.

Copy link
Member

@dennisdoomen dennisdoomen left a comment

Some general notes before I can continue the review:

  • I suspect a lot of the methods on the new extension classes are no longer needed
  • The unit tests need to be updated to take into account some of the comments we had on #1807

@jnyrup jnyrup added the feature label Feb 19, 2022
Copy link
Member

@dennisdoomen dennisdoomen left a comment

It's a big enhancement, albeit quite niche, so there is a lot of work to be done to finish this. I hope you're up for it.

FYI, just assume that my code review comment apply to all other code as well, so please be thorough.

logiclrd added 20 commits Apr 15, 2022
…llectionAssertionExtensions.cs, as their implementation is actually identical to that in GenericCollectionAssertions.

Reran AcceptApiChanges.ps1.
Added tests to DataRowCollectionAssertionExtensionsSpecs.cs to verify that ContainEquivalentOf and NotContainEquivalentOf do the right thing with DataRow objects & collections.
…ionAssertionExtensions.cs, DataColumnCollectionAssertionExtensions.cs and DataRowCollectionAssertionExtensions.cs.

Removed unnecessary calls to .ForCondition(false).
Removed unnecessary cast from IEnumerable<DataRow> to IEnumerable<object> in ContainEquivalentOf in DataRowCollectionAssertionExtensions.cs.
Updated ContainEquivalentOf in DataRowCollectionAssertions.cs to return an AndWhichConstraint.
…ionAssertionExtensions.cs and DataColumnCollectionAssertionExtensions.cs and the corresponding tests from DataTableCollectionAssertionExtensionsSpecs.cs and DataColumnCollectionAssertionExtensionsSpecs.cs.

Reran AcceptApiChanges.ps1.
Removed documentation for Contain and NotContain (by name) operations from data.md.
…aTableCollectionAssertionExtensionsSpecs.cs, DataColumnCollectionAssertionExtensionsSpecs.cs and DataRowCollectionAssertionExtensionSpecs.cs.

Corrected indentation error in DataRowCollectionAssertionExtensions.cs.
…aColumnCollectionAssertionExtensionsSpecs.cs and DataRowCollectionAssertionExtensionsSpecs.cs.
…d DataColumnCollectionAssertionExtensionsSpecs.cs. Since the tests don't actually look at the column specifications, it isn't important that they be assigned a variety of different types.
…ation DataTables in DataTableCollectionAssertionExtensionsSpecs.cs and DataRowCollectionAssertionExtensionsSpecs.cs.
….cs so that the DataSet overloads are covered.
- Removed unnecessary references to IEnumerable and IEnumerable<T>, already covered by implementing/constraining against ICollection and ICollection<T>.
- Made UnderlyingCollection in ReadOnlyNonGenericCollectionWrapper<T> immutable.
…llectionAssertionSpecs.cs.

Renamed DataColumnCollectionAssertionExtensionsSpecs.cs to DataColumnCollectionAssertionSpecs.cs.
Renamed DataRowCollectionAssertionExtensionsSpecs.cs to DataRowCollectionAssertionSpecs.cs.
…in TypeDescriptionUtilitySpecs.cs.

Refactored AssertionScope.FailWith to produce new utility method AssertionScope.FormatFailureMessage.
Updated DataTableCollectionAssertionExtensions.cs, DataColumnCollectionAssertionExtensions.cs and DataRowCollectionAssertionExtensions.cs to throw InvalidOperationException when BeSameAs or NotBeSameAs is called with a subject of the wrong type, using the new AssertionScope.FormatFailureMessage method to give the exception an appropriate message.
Updated DataTableCollectionAssertionSpecs.cs, DataColumnCollectionAssertionSpecs.cs and DataRowCollectionAssertionSpecs.cs to account for the change in the exception type & message in the When_generic_collection_is_tested_against_typed_collection_it_should_fail test methods.
…in DataTableCollectionAssertionExtensions.cs, DataColumnCollectionAssertionExtensions.cs and DataRowCollectionAssertionExtensions.cs into a standard assertion flow.

Updated unit tests correspondingly.
…an internal method FormatFailureMessage to allow reuse.
…othing _currently_ has a need to get a description for a Type directly.
…the TypeDescriptionUtilitySpecs.cs class correspondingly. All actual uses of TypeDescriptionUtility already import this namespace so no other changes are needed.
@logiclrd logiclrd force-pushed the JDG_DataSetCollections branch from d2076fd to 8c879bf Compare Apr 15, 2022
@logiclrd
Copy link
Contributor Author

logiclrd commented Apr 15, 2022

(rebased to resolve release.md conflict)

jnyrup
jnyrup approved these changes Apr 16, 2022
Copy link
Member

@jnyrup jnyrup left a comment

Sorry for the delayed review.
I gave it another look and it looks good.

@jnyrup jnyrup merged commit 6ecd349 into fluentassertions:develop Apr 16, 2022
1 check passed
@jnyrup jnyrup changed the title Restore basic assertions for collections in System.Data Restore basic assertions for collections in System.Data Apr 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants