Menu

#430 PrepAndExpectedTestCase should only sort on filtered columns

v2.6.*
pending
None
(not fixed)
5
2019-05-03
2019-04-11
Luc Feys
No

Hi,

Not sure if I'm missing something obvious, but it seems to me that PrepAndExpectedTestCase should only sort on the filtered columns, and not on all the columns.
Otherwise you might end up in a situation where testcases fail randomly (which actually happened for me).
Suppose the table for which you are verifying expected results contains a column with some randomly generated value as one of its first columns (e.g. aff_id).
You obviously want to ignore this column, so it is in the list of excluded columns and not in your expected dataset.
But if the datasets are sorted on all columns, then the actual dataset will be sorted on that excluded column, while expected results will not be, giving a potential (random) mismatch depending on the values of the 'random' column 'aff_id'.

Example:
Actual
aff_id | other_id | value
---------- | ---------- | ------
-315 | 25 | codeX
48 | 14 | codeY
Expected
other_id | value
---------- | ------
14 | codeY
25 | codeX

In the above example the verification will fail although - given 'aff_id' should be excluded - the datasets should be considered identitical.

I have tested with reversing the order of the logic in DefaultPrepAndExpectedTestCase#verifyData and filtering the columns first before sorting the tables. This seems to work fine for me, but I'm just wondering if I'm not missing something else.
If not, I could create a merge request containing the fix.

Regards,

Luc

Discussion

  • Jeff Jensen

    Jeff Jensen - 2019-04-19
    • status: open --> pending
    • assigned_to: Jeff Jensen
     
  • Jeff Jensen

    Jeff Jensen - 2019-04-19

    Hi Luc, thank you for the discussion on this.

    The current behavior of sorting on all fields defined in the dataset has existed since its creation. So changing it breaks backwards compatability (many users of dbUnit would have failed tests due to sorting on the first included column). It may not be desired this way tho, and can consider for change.

    The solution for the example you gave is to add an id column as first one in the expected dataset. dbUnit then ignores it on comparison and uses it for sorting. This behavior helps having datasets in same order for comparison, which is a fundamental requirement. Non-excluded columns may not have unique values to sort by.

    Ignoring columns is one of the key reasons for the ValueComparer feature creation - so we don't have to ignore columns - enabling comparing ID fields with comparisons such as "actual greater than or equal to expected".

     
  • Luc Feys

    Luc Feys - 2019-05-02

    Hi Jeff,
    Thanks for your reply.
    My problem is that the 'expected' datasets are being read from Excel files from a legacy testing framework. And we should still be able to run the legacy tests, so adding a column to the expected dataset could be causing problems (will have to investigate). Also there are a lot of those legacy files, so - even if adding a column would be possible - I want avoid making changes to them, since this will result in a lot of work.

    I understand that you cannot break existing functionality, so I will have to do some more research on how I can fix my issue, maybe by using the ValueComparer feature.

     
    • Jeff Jensen

      Jeff Jensen - 2019-05-02

      You have a good scenario, thank you for sharing.

      A solution idea is to allow the user to decide - add a feature toggle for performing this sort or not; default is on so is backwards compatible. You can turn it off. WDYT?

       
  • Luc Feys

    Luc Feys - 2019-05-03

    Yes, that would be perfect for my use case. I was kind of reluctant to propose this, since at first it felt somehow like patchwork.
    But giving it some second thought, it could indeed be presented to the user as a proper feature that is easy to understand with proper documentation.
    I will create a Merge Request.

     
    • Jeff Jensen

      Jeff Jensen - 2019-05-03

      Sounds good, thank you Luc.

      Please create a feature ticket for that work and we can close this one.

       

Log in to post a comment.