-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[sdk/nodejs] Implement getResource in the mock monitor #5914
Conversation
Otherwise, unit tests for programs that reference resources that have been registered with `registerResourceModule` fail with unhandled exceptions.
@pgavlin, if the approach in this PR looks good, I'll make similar changes for Python and the other languages. |
const tok = req.getTok(); | ||
const inputs = deserializeProperties(req.getArgs()); | ||
|
||
if (tok === "pulumi:pulumi:getResource") { |
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.
Presumably we are OK handling this on behalf of the user, so they don't have to implement it themselves.
If there ever was a scenario where it's interesting to allow mocking calls for "pulumi:pulumi:getResource"
in the future, I could imagine updating the Mocks
interface to support an optional getResource
function, that we could call if it was set instead of using the implementation here.
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.
We should be implementing this, as it is a builtin that deals only in state that is internal to the engine.
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
const tok = req.getTok(); | ||
const inputs = deserializeProperties(req.getArgs()); | ||
|
||
if (tok === "pulumi:pulumi:getResource") { |
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.
We should be implementing this, as it is a builtin that deals only in state that is internal to the engine.
Otherwise, unit tests for programs that reference resources that have been registered with
registerResourceModule
fail with unhandled exceptions.We didn't have any existing tests for mocks for Node.js, so I added some based on the ones we have for Python, and augmented it to cover this case. The new test fails before the change to the SDK, and passes afterwards.
Part of #5832