[8.5.1] Update skyframe error checking in case the second skyframe lookup in #28027
Conversation
ToolchainTypeUtils is incomplete. PiperOrigin-RevId: 702010313 Change-Id: I24983bd2a1c708f7f8f63ee87273e22ecddb3a72
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a potential NullPointerException in ToolchainTypeLookupUtil that could occur if a Skyframe lookup for a toolchain type was incomplete. The logic change is sound. However, instead of adding a regression test to prevent this issue from recurring, a TODO comment has been added. It is highly recommended to include the test with the fix.
| assertContainsEvent("no such package 'fake': BUILD file not found"); | ||
| } | ||
|
|
||
| // TODO: b/381396141 - Add a regression test for failure to find the second Skyframe value. |
There was a problem hiding this comment.
While adding a TODO is a good first step, it is strongly recommended to include the regression test for the bug fix in this pull request. This will prevent this NullPointerException from being reintroduced in the future. Without a test, the fix is not guaranteed to be permanent.
8bfa1e2
ToolchainTypeUtils is incomplete.
PiperOrigin-RevId: 702010313
Change-Id: I24983bd2a1c708f7f8f63ee87273e22ecddb3a72
Commit 7e4fb41