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

Display success message in Verification Suite Result #375

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

andreaslang
Copy link

#371

Display also a success message for verification, seeing the hint and assertOn value makes it easier to see which verification succeeded.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…assertOn value makes it easier to see which verification succeeded.
Copy link
Contributor

@twollnik twollnik left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution, we appreciate the changes. Just one clarification needed.

Comment on lines 127 to 132
"Success", "Value: 1.0 does meet the constraint requirement!"),
("group-2-E", "Error", "Error", "SizeConstraint(Size(None))", "Failure",
"Value: 4 does not meet the constraint requirement! Should be greater than 5!"),
("group-2-E", "Error", "Error", "CompletenessConstraint(Completeness(att2,None))",
"Success", ""),
"Success", "Value: 1.0 does meet the constraint requirement! Should equal 1!"),
("group-2-W", "Warning", "Warning",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you clarify why the message contains the hint ("Should ...!") in one case, but not in the other?

Copy link
Author

@andreaslang andreaslang Sep 9, 2021

Choose a reason for hiding this comment

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

Sure, the reason is that the first check does not actually have a hint. In that case only the standard message is shown.

The checks are in the same test suite from line 201:

private[this] def getChecks(): Seq[Check] = {
    val checkToSucceed = Check(CheckLevel.Error, "group-1")
      .isComplete("att1")

    val checkToErrorOut = Check(CheckLevel.Error, "group-2-E")
      .hasSize(_ > 5, Some("Should be greater than 5!"))
      .hasCompleteness("att2", _ == 1.0, Some("Should equal 1!"))

    val checkToWarn = Check(CheckLevel.Warning, "group-2-W")
      .hasDistinctness(Seq("item"), _ < 0.8, Some("Should be smaller than 0.8!"))

    checkToSucceed :: checkToErrorOut :: checkToWarn :: Nil
}

The change allows the message and hint to be passed through even if the evaluation was a success, before that the message and the hint was not displayed. Passing it through makes it a bit easier if the verification result is saved, because they can be used to know what the threshold at a given time was.

@twollnik
Copy link
Contributor

Thanks for clarification. One last request: can we change the message for the success case to "... meets the constraint requirement."? We think this would make it easier to distinguish between the success messages and the error messages.

@andreaslang
Copy link
Author

Sorry for the delay, was moving house. Makes sense, I updated the message.

@andreaslang
Copy link
Author

Thanks for clarification. One last request: can we change the message for the success case to "... meets the constraint requirement."? We think this would make it easier to distinguish between the success messages and the error messages.

Hi @twollnik , given that change is done, just wanted to check if you need anything else. Apologies again for providing the change for that a bit late.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants