aloof-angle-9161609/15/2020, 11:46 PM
, and wanted to share in general what I think he uncovered with this work (https://github.com/pantsbuild/pants/pull/10786#discussion_r489077220):
It looks like there's been a lot of work in init/logging.py on things that used to be in ExceptionSink. The only reason ExceptionSink was created was to centralize exception handling and formatting: see https://github.com/pantsbuild/pants/issues/6530. At this point I would actually advocate for potentially removing ExceptionSink entirely along with the skipped tests, as the current situation (which you're fixing) actually seems actively harmful and counter to the point of the ExceptionSink class in the first place (I can do that in a followup).
I have no clue how to avoid this happening in the future, but things like the backtrace printing twice as you mentioned on slack (https://pantsbuild.slack.com/archives/C0D7TNJHL/p1600140492122800) is exactly what I would expect if the logic was essentially duplicated elsewhere in pants. It feels like there's been some unnecessary code siloing going on that has led to directly recreating some of the same very tricky issues with error handling (because the point of ExceptionSink was to centralize error handling to fight back at exactly these issues).
In the future I would absolutely feel empowered to make these changes, and I think I would specifically recommend keeping an eye out for error handling in too many places if you ever feel like a refactoring.