-
Notifications
You must be signed in to change notification settings - Fork 100
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
feat: Update acceptance test report to include changes in WARNINGS. #1354
Conversation
✅ Rule acceptance tests passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
✅ Rule acceptance tests passed. |
Do we need to update https://github.com/MobilityData/gtfs-validator/blob/master/docs/ACCEPTANCE_TESTS.md as a result of this PR? |
…obilityData#1354) * Track changes to warnings in addition to errors in acceptance tests.
Refactor the acceptance test validation report comparison logic to consider changes to WARNING-level notices, not just ERRORs. We could more easily expand this to include INFO-level notices as well, if we so desire. Currently, this change does not cause automatic blocking of a change if the # of warnings changes significantly, but we could change that as well.
As part of this change, I refactored a bunch of logic out of
ValidationReport
and moved it into a newNoticeSet
to be separate some concerns. I also refactored a bunch of references to "errors" to more generic "notices".Closes #1349.
Please make sure these boxes are checked before submitting your pull request - thanks!
gradle test
to make sure you didn't break anything