Skip to content

Introduce a "validation report" into the validation suite #589

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

Merged
merged 14 commits into from
Dec 7, 2020

Conversation

sgsmob
Copy link
Contributor

@sgsmob sgsmob commented Dec 2, 2020

Description

Introduce a "validation report" object to the validation suite. This report handles the aggregation, printing, and interpretation of the errors raised by the various validation checks.

This not only separates the logic of aggregating the errors from the code that performs the checks but also should allow us to split out the validation checks into separate classes, using the report as an interface between them.

This includes changes introduced in #578 so it should not be reviewed until after that is merged.

Changelog

  • Add delphi_validation.report.ValidationReport to track the results of validation
  • Migrate existing validation code to use ValidationReport

@sgsmob sgsmob requested review from a team and chinandrew and removed request for a team December 4, 2020 15:19
Comment on lines 27 to 28
suppressed_errors: List[Exception]
Errors raised from validation failures not found in `self.errors_to_suppress`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be unsuppressed errors? Since it's errors that are not in the ignore list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, done.

"""
self.raised_warnings.append(warning)

def __str__(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a test that this formats to what we expect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added tests for the whole report class

@sgsmob sgsmob requested a review from chinandrew December 5, 2020 00:56
Copy link
Contributor

@chinandrew chinandrew left a comment

Choose a reason for hiding this comment

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

:shipit::shipit::shipit:

@krivard krivard merged commit acf2aef into cmu-delphi:main Dec 7, 2020
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.

3 participants