dbunit Merge Request #46: Extract database verification logic from PrepAndExpectedTestCase into separate VerifyOperation (open)

Hello,

As mentioned in ticket 425, I would like to have the logic that executes the verification of database tables against expected datasets (that currently resides in DefaultPrepAndExpectedTestCase) to be extracted into a separate Operation, so it can be used in any test flow, with being forced to use the PrepAndExpectTestCase flow.

So I did a first implementation of this:
* extracting all relevant logic into a class called VerifyOperation
* using that VerifyOperation class in DefaultPrepAndExpectedTestCase removing all (now) obsolete logic from that TestCase class
* splitting the unit test for DefaultPrepAndExpectedTestCase into an additional test for VerifyOperation
* and in the mean time making sure I did not break the public API of DefaultPrepAndExpectedTestCase (although some public methods - of which I am wondering why they were public in the first place - have been moved to VerifyOperation

Results
* All tests have been executed succesfully on my local machine
* I tested my own code which now uses the VerifyOperation and it works fine for me.

Some remarks
* Not particularly happy about the workaround for maintaining some of the public getters and setters on DefaultPrepAndExpectedTestCase. Would like to deprecate those
* Unit tests should be extended for VerifyOperation
* The setter for ExpectedDataSetAndVerifyTableDefinitionVerifier should be rather replaced by an additional parameter to the configureTest method

But before I continue to work on this, I would first like to hear what think of the idea and if it would be considere to be taken into the main development track.

[Jeff Jensen]

workaround for maintaining some of the public getters and setters

Deprecate the methods you suggest; we can discuss further when noted.

ExpectedDataSetAndVerifyTableDefinitionVerifier should be rather replaced by an additional parameter to the configureTest method

Changing configureTest breaks backwards compatibility; you can overload it tho.

I noticed your branch does not have the latest DefaultPrepAndExpectedTestCase changes.

Please rebase and squash adjustments. However, please keep separate focused commits for each class extraction (only 1 identified so far) and other improvements (e.g. deprecations, configureTest change).

[Luc Feys]

Thanks for the feedback:
I have updated my fork with latest changes from upstream (master) repository
Have set the public getters and setters deprecated and added a getVerifyOperation getter to provide an alternative to retrieve the same information
Have overloaded configureTest to allow specifying a ExpectedDataSetAndVerifyTableDefinitionVerifier
Have added all above mentioned optimizations into a separate commit (I hope I understood your request correctly here)
* Have aligned commit messages

[Jeff Jensen]

Thanks.

Have added all above mentioned optimizations into a separate commit (I hope I understood your request correctly here)

Yes, thanks for keeping refactorings separate - one commit for extracting verify operation (perhaps "F225: Extract VerifyOperation") and another for refactoring configureTest (perhaps "F225: refactor DefaultPrepAndExpectedTestCase.configureTest"). And any further refactorings...

[fce5ee] First commit

• DefaultPrepAndExpectedTestCase.java diff shows all lines as changed; please fix (line endings?).
• You can delete get|setExpectedDataSetAndVerifyTableDefinitionVerifier. They were recently added and unlikely in use (an easy change if either were).
  
  

[700423] Second commit

• JavaDoc correctness requires sentences, so please add periods.
• Fix minor formatting (@Override public void configureTest()).
• Looks like should move the get|setVerifyOperation methods to the first commit as it's part of the extraction(?).
  
  

VerifyOperation

I'm curious as to your design/plans/thinking with having VerifyOperation extend AbstractOperation. (it obviously lines up with setup/teardown operations, wondering if you have further ideas)

At some point, we'll need doc page changes as well. What are your thoughts for those?

[Luc Feys]

• Will have a look at your remarks with regard to commit-1 and commit-2. Not sure about the 'all lines difference' for 'DefaultPrepAndExpectedTestCase' it shows indeed CRLF as line endings, but this is the same for all files I checkout. Git converts the line endings again when I commit.

• About the design for VerifyOperation: when I started on this, I did not really know what functionalities would be needed from the base Operation classes, so I started from AbstractOperation, but it turned out I did not really need any of the functionalities in that base class.

• In fact I think the only thing that is needed is an execute method that receives a DatabaseConnection and DataSet as input parameters.
  This operation does not make any changes to the database, so does not really line up with the Insert/Update/Delete operations that extend AbstractOperation. So I guess it could just as well extend DatabaseOperation, in order for it to have the same 'execute' API as the other Operations.

• I also thought about somehow making a difference between 'readonly' (Verify) and 'modifying' (Insert/Update/Delete) database operations, but I see no distinctive differences between them with regard to exposed API, so not sure that would add any value.

• Not sure what you mean with 'my thoughts on the doc page changes'. Should the docs not just describe the provided functionality?

Will be picking this up again next week. Have nice weekend.

[Jeff Jensen]

Not sure about the 'all lines difference' for 'DefaultPrepAndExpectedTestCase'

Thanks for checking... I no longer see the problem so that's good. I don't know what happened...

I started from AbstractOperation, but it turned out I did not really need any of the functionalities in that base class.

That's my thinking, but wasn't sure of your plans. Probably better to not link them; do it in the future if a reason found.

only thing that is needed is an execute method that receives a DatabaseConnection and DataSet as input parameters.

See what the final need is and adjust accordingly.

making a difference between 'readonly' (Verify) and 'modifying' (Insert/Update/Delete) database operations, ... not sure that would add any value.

Agreed. Start without it and see if something realizes.

Should the docs not just describe the provided functionality?

Yes... I was thinking of where and what. e.g. extracting Verifier can mean it is a core component, usable by others, therefore perhaps on the components page (?). But depending, we may want to create a new page as well. I try to plan docs along with the feature/fix so it isn't an afterthought or missed. Sometimes, working on the docs along with the change enables discovering other facets to include or avoid in the code.

Further, (regarding my refactoring DefaultPrepAndExpectedTestCase comment on the feature ticket), my initial notes are to extract some or all of these:

• PrepAndExpectedTestCaseConfigurer
• PrepAndExpectedTestCaseRunner
• PrepAndExpectedTestCaseVerifier
• PrepAndExpectedTestCaseCleanup
• data loaders?

Creating these interfaces enables a default implementation and then easy substitution of parts by anyone as needed; Strategy pattern for them.

Verifier may be the only one we can make and name generically, using it outside of just PrepAndExpectedTestCase. I haven't analyzed, perhaps Cleanup could as well.

So deciding packages, class names, and doc locations (augmenting and new) help define intentions/what we should do, and therefore the feature and the path to take.

[Luc Feys]

I have implemented all proposed modifications to the two commits, with the following remarks:
Added some ITs for VerifyOperation in the first commit
Only removed get|setExpectedDataSetAndVerifyTableDefinitionVerifier in the second commit, since the refactor of configureTest is required in order to make the setter obsolete

Some additional thoughts/questions:
Why are some of the methods in VerifyOperation 'public' (like applyColumnFilters, loadTableDataFromDataSet, ...). They seem pretty generally usuable, so was it perhaps the intention to extract them to some helper class?
The VerifyOperation now contains quite some (protected/private) logic that could maybe be grouped and extracted into a few smaller helper classes that could easily be unit tested. The tests currently in VerifyOperationTest could then move to the test class of such a helper class.

About your questions:

extracting Verifier can mean it is a core component, usable by others, therefore perhaps on the components page

I see 'VerifyTableDefinition' and 'ValueComparer' are already core components, which are closely related to the VerifyOperation, but I somehow feel like VerifyOperation belongs on a higher level. The classes/interfaces in core components are quite low level building blocks, which VerifyOperation is not.

Creating these interfaces enables a default implementation and then easy substitution of parts by anyone as needed; Strategy pattern for them.

That sounds like a good idea, although I personally did not use PrepAndExpectedTestCase yet, as my main focus so far has been on the verification logic only. So - besides the extraction of the Verification logic - I have no suggestions yet for potential improvements or limitations of the current approach that I ran into.

deciding packages, class names, and doc locations (augmenting and new) help define intentions

packages:
- if PrepAndExpectedTestCase get's split up in multiple interfaces and default implementations, it will definitely deserve it's own package (maybe 'org.dbunit.testcase.verify' or 'org.dbunit.testcase.expected'?). There are already quite some classes involved now.
- Same goes for VerifyOperation if it would be split up in a few more classes. Maybe 'org.dbunit.operation.verify'? It could turn out later then that some of the helper classes can be put to a wider use.

docs
- I woud think the docs for VerifyOperation would rather fit together with the 'Data Comparison' section, maybe under a broader topic related to 'expected data verification'?

[Jeff Jensen]

Well I certainly let you down on this one. I apologize...
If you are still interested in this, please let me know as my time is better now. Understood if you are not; I will move forward with PrepAndExpectedTestCase improvements and will include your ideas and work on this one.