[CSHARP-1029] Introduce interfaces for IBsonReader/IBsonWriter Created: 12/Aug/14 Updated: 02/Apr/15 Resolved: 03/Dec/14 |
|
| Status: | Closed |
| Project: | C# Driver |
| Component/s: | BSON |
| Affects Version/s: | 1.9.2 |
| Fix Version/s: | 2.0 |
| Type: | Improvement | Priority: | Major - P3 |
| Reporter: | Svetoslav Milenov | Assignee: | Robert Stam |
| Resolution: | Done | Votes: | 0 |
| Labels: | bson | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Backwards Compatibility: | Fully Compatible |
| Description |
|
To enable easier testing of serializers (along with other benefits), interfaces should be introduced where BsonReader and BsonWriter are currently being used. Old If the custom serializer depends on the CurrentBsonType or State properties, it can not be tested easily, as these properties are not virtual, and have protected setters. There is an ugly workaround to create your own abstract inheritors of the above classes, where one can provide a public method to set these properties, and then for the test to mock that inheritors. A better approach would be to make these properties virtual, providing for a better user code testability. |
| Comments |
| Comment by Githook User [ 03/Dec/14 ] |
|
Author: {u'username': u'rstam', u'name': u'rstam', u'email': u'robert@robertstam.org'}Message: |
| Comment by Craig Wilson [ 12/Aug/14 ] |
|
We have discussed changing these to interfaces as well, just haven't pulled the trigger. Too many other, more important things to do. While we can certainly debate the philosophy of what a pure unit test is, our current ones don't require any extrenal dependencies other than a working BsonWriter/BsonReader. And since those can be instantiated based on byte arrays or from parsing json fairly easily, we believe it much more clear to be testing with actual implementations of these classes. I do, however, understand your dilemna that all you want to do is test something small. So... I don't think making these virtual is the right thing. The right thing is to use interfaces for BsonWriter/BsonReader. We are not at a point where we are able to discuss this change, so please don't go and do this. I am going to change this ticket's description to Introduce interfaces for BsonReader and BsonWriter to remind us this is important. |
| Comment by Svetoslav Milenov [ 12/Aug/14 ] |
|
Hi Craig, I checked the tests you mention, but they are not "pure" Anyway, the change I proposed is the least impact on the code base. A proper solution would be to change the signature of IBsonSerializer methods to IBsonReader, and IBsonWriter, and make BsonWriter and BsonReader abstract classes to implement these interfaces. That way there will be a better decoupling. But this was a much bigger change, affecting too many files. If you think such a change is acceptable. I can change my pull request and implement it. |
| Comment by Craig Wilson [ 12/Aug/14 ] |
|
Hi Svetoslav, Thanks for the PR and suggestion. Unfortunately, there is a lot of behavior in these base classes and mocking them to test your implementation wouldn't prove much. I'd suggest looking at how our internall implemented serializers are tested, and do that. Since they don't integrate with anything (mongodb, files, etc...), this isn't a problem provides a much more robust and provable implementation. Craig |
| Comment by Svetoslav Milenov [ 12/Aug/14 ] |
|
Pull request added: https://github.com/mongodb/mongo-csharp-driver/pull/186 |