Uncomment test_csv_export_with_nonexistent_table #2677
Assignees
Labels
Milestone
Comments
DeltaMichael
added
enhancement
DeltaMichael
added a commit
that referenced
this issue
Sep 21, 2023
…ation (#2666) ## Why As part of the run logs initiative, we should add classification attributes to exceptions that are handled in vdk. These attributes are used for easier error classification and will help us enrich the vdk exception class family, so we can pass more info to our users. This is not possible without separating the error logging and error reporting mechanisms, so some refactoring was required. ## What The main changes are in `errors.py`. All other changes (tests, other modules) are a result of changes in `errors.py`. The changes for specific plugins are out of scope for this PR due to compatibility issues. ### Add attributes to exceptions Base vdk exceptions now have two new attributes - `to_be_fixed_by` and `type` (names subject to change). These help the error classification functions determine who to blame when an exception is raised. They're also displayed when pretty-printing the vdk exceptions. UserCodeError, VdkConfigurationError and PlatformService error are preserved as convenience classes that hardcode these attributes, based on who we decide is responsible for these exceptions. They can be extended into more specific exceptions in subsequent PRs, related to #2572 ### Introduce a reporting API ``` report(error_type, exception: BaseException) report_and_throw(exception: BaseVdkException) report_and_rethrow(error_type, exception: BaseException) ``` The report functions adds any base exception (not necessarily a vdk one) to the resolvable context. It determines who to blame for the error and sets the `to_be_fixed_by` and `type` attributes of the exception based on the error classification. report_and_throw is used for convenience. It adds the vdk exception that was passed to the resolvable context and throws it. report_and_rethrow just calls report and throws the exception that was passed. Finally, we have the `log_exception(log, exception: BaseException, *lines)` function. It builds a message from the lines we pass and logs it at the warn level. It then logs the exception that was passed. The result is that we've effectively decoupled logging from the error reporting and error classification functions. We can choose to report an exception and not log it, or log and exception and not report it, depending on how we want to handle it. This provides a certain amount of freedom if we want to eliminate log statements we consider unnecessary. ### Change the way vdk exceptions are represented Override the __str__ and __repr__ methods in BaseVdkException. The constructor takes a variable number of lines that are used to create the error message and exception representation. We print a nice box that tells us what kind of exception occurred and who should fix it, along with the details. The constructor can still take an ErrorMessage or dict instance for compatibility reasons. That logic should be removed when we remove ErrorMessage from the plugins. ### What was removed Passing `ErrorMessage` to vdk exceptions is not necessary anymore, so the pattern has been removed wherever it's used for instantiation in vdk-core. Formatting the error message also had separate functions, which are no longer needed, so they've been removed. The decorator for error messages for some special cases has been removed and the logic added to the only function that actually used it. DomainError, `log_and_throw` and `log_and_rethrow` are kept for compatibility reasons, but they should be phased out in all plugins that call them as separate PRs. ## Resulting user experience **Before:** https://pastebin.com/raw/YsgzpUj4 Configuration error is printed with verbose stack trace **After:** https://pastebin.com/raw/MUsLcjB2 Configuration error is printed with shorter stack trace **Before:** https://pastebin.com/raw/qbfRfAUn Exception coming from user code is wrapped in UserCodeError **After:** https://pastebin.com/raw/1HGfDqye Exception coming from user code is not wrapped, but attributes are set correctly, e.g. blamee is displayed as user error in the subsequent result. **Alternative**: https://pastebin.com/raw/AsjDeyHZ Alternatively, we can wrap non-vdk exceptions on re-throw, but this means we should re-visit some of our logging statements, because the logs become too verbose. Since logging exceptions is now separate from reporting them, we can dive deeper into the issue. ### Folow-up #2677 #2678 #2679 #2680 #2681 #2682 #2683 #2684 ## How was this tested Ran locally with jobs that cause errors, ran the vdk-core tests locally as well. For plugin tests, we rely on CI. ## What kind of change is this Potentially breaking change Signed-off-by: Dilyan Marinov <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Overview
test_csv_export_with_nonexistent_table was commented, because it blocked merging of #2666. It was executing both against the current version vdk-core in PyPi and the changes in the PR. The PR changes the type of error that's thrown, so the test has no chance to pass both times.
Acceptance criteria
The text was updated successfully, but these errors were encountered: