-
Notifications
You must be signed in to change notification settings - Fork 27
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
Enhance Error handling #163
Conversation
* Simplify tenant metadata section and correct groups bug * Remove scope that is no longer needed * Remove unneeded OAUTH scope from readme
@rgbrow1949 @LaurenBassett Tagging you for awareness, as this is a significant change that everyone should be aware of. |
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.
Comments
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.
Conditionally approving as the code is functional at the time of my remove. Have a comment below to address before merging.
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.
Approving these, but definitely want to go over all the changes in our weekly.
🗣 Description
💭 Motivation and context
Closes #157.
Additionally, before this update, if an API call or function failed, the report could display inaccurate information (as evidenced by #147). The terminal would display a warning in cases like this, but it would be better to prevent the inaccuracies from being saved to the report.
The method I implemented for this mirrors the strategy used for ScubaGear.
See the following snippet for an example of what step 2 looks like:
At the moment, the vast majority of our tests just depend on the reports API, reports/v1/activities/list, the exceptions being:
Given that, to reduce code duplication, I made it so that if "Prerequisites" isn't defined for a given test, the Reporter just assumes dependence on
reports/v1/activities/list
. As Google's API matures, we'll be able to develop more granular prerequisites for each test.📷 Screenshots (if appropriate)
Examples of what the enhanced error handling looks like in the reports:
🧪 Testing
Note that I did not add the missing checks for group 19, "Attachment Compliance Filtering." That is because that group is being deleted by #156. In the meantime, that means you can run ScubaGoggles and see the new error handling in practice. Once both PRs are merged to main, the warnings will go away.
An easy way to test the error handling is to hard-code an error into the code (like
x/0
).✅ Pre-approval checklist
✅ Pre-merge Checklist
Squash and merge
button.✅ Post-merge Checklist