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

Improve failure message of RowCountPrecondition to preserve expected row count #3093

Merged
merged 2 commits into from Aug 1, 2022

Conversation

martinspielmann
Copy link
Contributor

Impact

  • Bug fix (non-breaking change which fixes expected existing functionality)
  • Enhancement/New feature (adds functionality without impacting existing logic)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

The existing failure message for RowCountPrecondition will always tell Table [NAME] is not empty., even if there is an expected row count greater than zero.

E.g. you set expectedRows to 5 and your table has 10 rows, you'll get the following failure message, which is somehow misleading, because you do not expect to have an empty table at all:
Table [NAME] is not empty.

The change updates the error message to be more specific and contain the expectedRows:
Table [NAME] does not have the expected row count of 5.

Things to worry about

  • I'm afraid this very small change changes the method signature of getFailureMessage, so classes extending from RowCountPrecondition may in fact break if they did override this method.

@kataggart kataggart added this to To Do in Conditioning++ via automation Jul 21, 2022
@nvoxland nvoxland added the SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions label Jul 27, 2022
@nvoxland nvoxland self-requested a review as a code owner July 27, 2022 18:34
@nvoxland nvoxland added SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions and removed SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions labels Jul 27, 2022
@github-actions
Copy link

Unit Test Results

  4 620 files  ±0    4 620 suites  ±0   35m 47s ⏱️ + 1m 37s
  4 594 tests ±0    4 375 ✔️  - 4     219 💤 +4  0 ±0 
54 300 runs  ±0  49 276 ✔️  - 4  5 024 💤 +4  0 ±0 

Results for commit 0a4c5f6. ± Comparison against base commit 28631d9.

Copy link
Contributor

@nvoxland nvoxland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review and test results:

Things to be aware of:

  • Change makes sense to me
  • No test, but I can't think of a useful test to add at this point
  • I'm OK with the minor API change

Things to worry about:

  • Nothing

@nvoxland nvoxland requested a review from suryaaki2 July 27, 2022 22:10
@FBurguer
Copy link

FBurguer commented Aug 1, 2022

Precondition work as expected, it shows how many rows you have vs expected rows. If it doesnt match, changset its not executed/warns you (depends on how you set up the precondition).

@nvoxland nvoxland merged commit 9c4e585 into liquibase:master Aug 1, 2022
Conditioning++ automation moved this from To Do to Done Aug 1, 2022
@kataggart kataggart added this to the v4.15.0 milestone Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errorReporting SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions sprint2022-30
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants