dbunit Merge Request #47: Case sensitivity fixes in filtered data sets and database data set schema sets (merged)

[Jeff Jensen]

Hi, thank you for the MR.

Looks like you have 3 fixes? One MR for all 3 is fine however, please create 3 separate bug tracker entries (or 2 if you want to combine the case sentitivity fixes into one).

Are you able to add small tests to prove these errors, and therefore preventing them from happening again?

Further discussion for the third commit, IsActualWithinToleranceOfExpectedTimestampValueComparer:

1. What database and column type caused this problem for you?
2. What was the actual error/exception occurring for your situation?
3. I'm very interested in tests that reproduce this problem. Are you able to create please?
4. Is it correct that dataType.typeCast(actualValue)handles the situations so that the (Timestamp)downcast in convertValueToTimeInMillis always works (at least for your situation)?
5. Please don't use compound statements - extract dataType.typeCast(actualValue) and dataType.typeCast(expectedValue) to local variables (simple statements greatly helps with any NPE diagnosis).
6. We shouldn't swallow the TypeCastException because it is an error and and the test should not pass if this happens so please remove the try/catch.

---

Until we complete our "contributing" doc page, the following are notes to help with your contribution process.

Please update your commits accordingly and let me know when ready for another review or further discussion. Let me know any questions, suggestions, etc.

== Tracking Changes
If one does not already exist, create a tracker entry, bug or feature, for the change.

== Keep Current with Master
Contribute changes using merge requests (preferred) or patches attached to tracker entries. Rebase your changes on master as needed, especially when ready for review and potential merge to master.

== Update changes.xml
Add an entry for the change to changes.xml. Include a terse summary at the end of description. See existing entries for examples. Include this file in the commit with the code changes.

== Commit Messages
Follow git standard good commit message practices. Good references on this include:

https://chris.beams.io/posts/git-commit/#seven-rules
https://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
https://wiki.openstack.org/wiki/GitCommitMessages

Additionally, prefix the commit topic with F (for feature) or B (for bug) and the tracker id, e.g. Fnnn or Bnnn. See the commit history for examples.

== Discussions and Questions
For questions, assistance, or other help, please comment on the tracker item, merge request, or email the dev list.

[Luke Cann]

Hi Jeff,

I've raised bugs for all three of the fixes:
426: FilteredDataSet does not inherit case sensitivity of wrapped set
427: DatabaseDataSet SchemaSet is incorrectly normalizing schema names in regard to case sensitivity
428: IsActualWithinToleranceOfExpectedTimestampValueComparer assumes values are represented as Timestamps

I'll implement tests for each one and commit with the relevant bug reference.
I've done 426 and i'll get to the others soon.

1. This issue came about when i was trying to use this comparer with a untyped xml expected set against a database set from a MSSQL DB.

2.
The error i experienced was a ClassCastException on the (Timestamp) cast in convertValueToTimeInMillis.
This happened for the expected value which was a String since it had come from the XML data set.

3.
Sure, i'll try to get around to this a little later

4.
I expect so, and i've not had any further problems with it so far.
The only situation i can think of where this would break is if this comparer is used on a column which is not a Timestamp.
I should imagine this is acceptable given that this checker is specifically for use with Timestamps.
A check could be added in the 'isExpected' to ensure the dataType is Timestamp for a more friendly error.

5/6.
I've made these changes, thanks for the feedback.

[Jeff Jensen]

Thank your for creating those, making the updates, and answering all my questions! The tests will especially help with understanding and long term quality.

1. Are you using DefaultPrepAndExpectedTestCase? If so, are you able to try the failing app tests with the published dbUnit 2.6.1-SNAPSHOT (dbunit.sourceforge.net)?
   It has commit [0066f5] which uses the actual dataset types when expecteds are unknown (which is usually true!) and probably fixes the issue. The specific commit note for it:
   "When the expected dataset's column definitions are UNKNOWN, use the actual dataset's column definitions for it so sorting and data comparisons use defined datatypes instead of defaulting to Strings."

2. I think the issue won't happen anymore with commit [0066f5] and like to know if it solves the problem in your scenario without this MR change. We can still apply this MR regardless.

3. Wonderful, thank you.

4. OK great. Agreed, it's for Timestamps. Exceptions with helpful error messages are Very Good Things™.

5/6. Thanks.

[Luke Cann]

Unfortunatly i am using Assertion.assertWithValueComparer directly instead of DefaultPrepAndExpectedTestCase so that commit doesn't fix the issue for me.

I've added tests to 426 & 428 but i'm having a hard time with 427.
I wanted to add the test to DatabaseDataSet_MultiSchemaTest but the issue doesn't seem to be reproducable with the H2 DB. I'll try again later with the mssql ITs as that is where i initially found the issue.

[Jeff Jensen]

Thank you for the updates. Any success with 427's test?

Some notes:

• Just ensuring you know that master has had some updates, so rebase when you are ready.
• changes.xml: please put your entries at the top instead of the bottom (newest at the top), and update the description attribute with a terse summary.
• You can squash the updated commits to their respective ones when ready.
• Would appreciate your feedback on the updated developers guide: http://dbunit.sourceforge.net/devguide.html

[Luke Cann]

Hi Jeff,
I was eventually able to write a test for 427. I've rebased on master and squashed my changes down to three commits, one for each issue. This includes the changes to changes.xml that you requested.

I looked through the dev guide. It seems to contain everything you've walked me through here at-least. It may be usefull to have some notes on code style. Most things, such as brackets and indentation is pretty clear at first glance but other things may not be.

[Jeff Jensen]

Thank you, very good. Merged.
For future ones, note that I updated your commits with master rebase and format corrections (tabs vs spaces, wrapping).