Mock resource monitor #3738

pgavlin · 2020-01-12T22:44:34Z

These changes add support for mocking the resource monitor to the NodeJS
and Python SDKs. The proposed mock interface is a simplified version of
the standard resource monitor that allows an end-user to replace the
usual implementations of ReadResource/RegisterResource and Invoke with
their own. This can be used in unit tests to allow for precise control
of resource outputs and invoke results.

pgavlin · 2020-01-12T22:47:35Z

Usage examples for Node and Python: https://gist.github.com/pgavlin/8904a129d1c5e826606ca8482b6386bd

pgavlin · 2020-01-12T22:48:27Z

@mikhailshilkov / @CyrusNajmabadi could one of you take a stab at implementing this for .NET?

EvanBoyle

Looks great! Longer term, I wonder if the schema work could help us generate and publish mock resources: https://gist.github.com/pgavlin/8904a129d1c5e826606ca8482b6386bd#file-ec2tests-js-L8-L17

ringods · 2020-01-14T14:26:06Z

@pgavlin nice work. I recognize a lot of my ideas I passed on to @stack72 as a slide deck. If you haven't seen these slides, ask Paul.

mikhailshilkov · 2020-01-14T16:04:59Z

.NET work is in #3696

mikhailshilkov · 2020-01-16T15:48:13Z

I'd love to see an example in TypeScript to see how much type safety is available.

dustinfarris · 2020-01-16T16:19:11Z

I'll be trying this out in TypeScript (via ts-jest) today.

We currently use mocha+sinon to accomplish a lot of what this PR does. Pain points have been:

Import order is tricky making test scaffolding brittle. Stubs/mocks have to be in place before a module and its dependencies are loaded. The way mocha discovers and loads all test files before execution makes things even weirder. This problem has really compounded for us as the project and its inter-dependencies have grown. We are considering moving all of our code into a combination ofbuild() functions and dependency-injection. We tried this before, and reverted back to the script-like style because dependency-injection gets complex as the project grows. Hoping that Pulumi can work out a style recommendation here.
Tests are slow because modules and their dependencies have to be reloaded often. node require caching is hard to understand and causes weird issues. To work around this, we often delete from require.cache and re-require various modules. This slows things down a lot.
Tests are slow whenever there is a lambda callback present. Lambda serialization is non-trivial. Because we have to reload modules often to set mocks/stubs/spies, the callbacks are re-serialized frequently which slows down the test run.
Certain computed outputs are not supported in "test mode". aws.glue.Database.name, for example, is computed by Pulumi, but is not available to inspect when testing.
Invoke errors whenever a resource needs to read from the AWS API. e.g. getXYZ() methods fail in unit test mode #2921

Will report back on our testing experience using this branch.

cc @joeduffy

dustinfarris · 2020-01-16T21:51:58Z

Can someone give me a hand with installing this branch locally? The pulumi nodejs sdk README instructions are not working for me.

EvanBoyle · 2020-01-16T21:58:29Z

Can someone give me a hand with installing this branch locally? The pulumi nodejs sdk README instructions are not working for me.

@dustinfarris you should be able to build pulumi/pulumi from these directions https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md

dustinfarris · 2020-01-16T22:16:38Z

Can someone give me a hand with installing this branch locally? The pulumi nodejs sdk README instructions are not working for me.

@dustinfarris you should be able to build pulumi/pulumi from these directions https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md

I followed the instructions, running make ensure and then make. make complains about tsc not being installed, which makes sense because none of the npm dependencies are installed. I took liberty to run npm install even though that is not explicitly mentioned in README or CONTRIBUTING, but that is failing with:

[I] ~/D/p/s/nodejs (pgavlin/mocks|…) [2] > npm install
npm WARN Invalid version: "${VERSION}"

Am I missing a step?

EvanBoyle · 2020-01-16T22:19:57Z

Was this after running the command to install dependencies?
https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md#getting-dependencies-on-macos

brew install node pipenv python@3 typescript yarn go@1.13 golangci/tap/golangci-lint
brew cask install dotnet dotnet-sdk

dustinfarris

Without any other changes, pulling in this branch caused pulumi.all.apply calls in our tests to start yielding undefined for outputs.

dustinfarris · 2020-01-17T22:50:39Z

sdk/nodejs/runtime/mocks.ts

+
+export interface Mocks {
+    call(token: string, args: any, provider?: string): any;
+    newResource(type: string, name: string, inputs: any, provider?: string, id?: string): { id: string, state: any };


Is it expected that newResource will need to have a return value for all resources within the program?

Yes - though I believe you can just return fixed id and no state for anything you haven't yet (or don't plan to) fully mocked.

Curious - what would you want to happen for resources you haven't mocked here?

what would you want to happen for resources you haven't mocked here?

I think returning an ID and copying inputs to outputs would be a sensible default

lukehoban · 2020-01-18T00:02:29Z

pulling in this branch caused pulumi.all.apply calls in our tests to start yielding undefined for outputs.

What version of @pulumi/pulumi were you using previously? There's nothing in this PR that should affect that. But there have been other changes in the last 6 months that might impact this. Can you share a code snippet of where you see a behaviour change?

dustinfarris · 2020-01-21T21:31:35Z

pulling in this branch caused pulumi.all.apply calls in our tests to start yielding undefined for outputs.

What version of @pulumi/pulumi were you using previously? There's nothing in this PR that should affect that. But there have been other changes in the last 6 months that might impact this. Can you share a code snippet of where you see a behaviour change?

Our prod is currently on 1.7.1.

Turns out this was a type error on our end that was somehow getting swallowed.

mikhailshilkov · 2020-01-22T14:00:22Z

I made the example work in my environment and using TypeScript. Several random observations:

Works great! I get the expected successes and failures.
call function is required in the mock that I pass to setMocks (made it empty for now).
The runner was failing with missing supportsFeature function on MockMonitor, so I had to add it too.
At first, I made a mistake of moving import * as infra from "./index"; to the top of the file before the call to setMocks and was wondering why tests mostly worked but not completely. We should help users avoid this error.
Tests themselves are nicely strongly typed. The use of pulumi.all makes them a bit harder to read, but not too bad.
I don't like the fact that we had to export the resources in order to test their properties. That makes the tests feel more coupled to the implementation. Say, if people move resources to a component resource, they would have to re-write tests. I'm not able to test resources that aren't exported. Ideally, I would be able to inspect the whole tree of resources in any case.
We could probably provide the default implementation of the mocks and let users override it if needed.
Mocks are very much untyped, so it's all hard-coded magic strings and any's, which makes them hard to write and error-prone.

I would love to discuss this together with my two .NET implementations in #3696

dustinfarris · 2020-01-22T14:08:59Z

Tests themselves are nicely strongly typed. The use of pulumi.all makes them a bit harder to read, but not too bad.

I was able to make this a little nicer by writing a helper that promisifies .apply:

const getOutputs = (desiredOutputs: pulumi.Output<any>[]): Promise<any[]> => {
    return new Promise(resolve => {
        pulumi.all(desiredOutputs).apply(resolve);
    });
};

it("is easier to read and easier to write", async () => {
    const [bucket] = await getOutputs([infra.myBucket.bucket]);
    assert.equal(bucket, "my-bucket");
});

pgavlin · 2020-01-22T17:03:10Z

I don't like the fact that we had to export the resources in order to test their properties. That makes the tests feel more coupled to the implementation. Say, if people move resources to a component resource, they would have to re-write tests. I'm not able to test resources that aren't exported. Ideally, I would be able to inspect the whole tree of resources in any case.

Just to make sure I understand correctly, you're referring to the need to export resources from the TS file, right?

mikhailshilkov · 2020-01-22T17:07:41Z

Just to make sure I understand correctly, you're referring to the need to export resources from the TS file, right?

Yep

sdk/nodejs/runtime/settings.ts

pgavlin · 2020-01-28T18:45:39Z

Are we confident enough in this approach that we can merge this?

mikhailshilkov · 2020-01-28T19:05:40Z

@pgavlin Any feedback on #3738 (comment)? Also, I'd love to sync with .NET before we merge, including questions in #3696 (comment)

dustinfarris · 2020-01-28T21:54:22Z

sdk/nodejs/runtime/mocks.ts

+const structproto = require("google-protobuf/google/protobuf/struct_pb.js");
+
+export interface Mocks {
+    call(token: string, args: any, provider?: string): any;


What is call() for? I thought it might be a way to mock things like aws.getCallerIdentity or secretsmanager.getSecretVersion but doesn't appear to work that way.

dustinfarris · 2020-01-28T22:04:11Z

Are we confident enough in this approach that we can merge this?

@pgavlin feeling good about the approach — this goes a long way toward alleviating our pain points.

A few gaps we've seen:

Loss of type-safety in newResource return values. There may be a neat TS trick we aren't aware of to achieve this.
"data source"-esque functions still need to be mocked by other means.
pulumi.StackReference outputs need to be mocked by other means.

I don't think any of these are show-stoppers. This PR as-is makes our test suite a lot nicer. Thank you for this.

pgavlin · 2020-01-29T19:28:23Z

"data source"-esque functions still need to be mocked by other means.

These should be mockable by implementing call. Is that not working for you?

pgavlin · 2020-01-29T19:29:00Z

pulumi.StackReference outputs need to be mocked by other means.

This is also surprising--pulumi.StackReference should end up calling in to newResource in the mocks.

dustinfarris · 2020-01-29T19:32:56Z

"data source"-esque functions still need to be mocked by other means.

These should be mockable by implementing call. Is that not working for you?

pulumi.StackReference outputs need to be mocked by other means.

This is also surprising--pulumi.StackReference should end up calling in to newResource in the mocks.

Ok, I'll try both of these again to rule out user error.

dustinfarris · 2020-01-30T16:53:14Z

Mocking pulumi.StackReference still does not seem to be working for me. For example, we have the output crossAccountRole defined on one of our stacks, which we retrieve in this project:

export const otherStack = new pulumi.StackReference(
    "our-org/other-project/other-stack",
);

Then, using setMocks to mock the output:

pulumi.runtime.setMocks({
    newResource: function(type, name, inputs) {
        if (
            type === "pulumi:pulumi:StackReference" &&
            name === "our-org/other-project/other-stack"
        ) {
            return {
                id: "",
                state: {
                    crossAccountRole: "fake-arn",
                },
            };
        }
    // ...

Then, trying to test that this mock is working:

it("has a fake output", done => {
    otherStack.getOutput("crossAccountRole").apply(role => {
        assert.equal(role, "fake-arn");
        done();
    });
});

throws this error:

    TypeError: Cannot read property 'crossAccountRole' of undefined

      at ../../../../opt/pulumi/node_modules/@pulumi/pulumi/stackReference.js:62:96
      at ../../../../opt/pulumi/node_modules/@pulumi/pulumi/output.js:220:35
      at ../../../../opt/pulumi/node_modules/@pulumi/pulumi/output.js:21:71
      at Object.<anonymous>.__awaiter (../../../../opt/pulumi/node_modules/@pulumi/pulumi/output.js:17:12)
      at applyHelperAsync (../../../../opt/pulumi/node_modules/@pulumi/pulumi/output.js:199:12)
      at ../../../../opt/pulumi/node_modules/@pulumi/pulumi/output.js:179:65

I added some logging and confirmed that newResource is getting triggered for the stack reference resource, but for some reason getOutput still throws this error.

dustinfarris · 2020-01-30T17:32:26Z

"data source"-esque calls are hanging for me. If I use setMocks like this:

pulumi.runtime.setMocks({
    newResource: function(type, name, inputs) { /* ... */ },
    call: function(token: string, args: any, provider?: string) {
        return "foobar";
    },
});

and try to test it:

it("is mocked", () => {
    assert.equal(
        aws.secretsmanager.getSecretVersion({ secretId: "foo" }),
        "foobar",
    );
});

The test hangs with no output. I also tried this with aws.getCallerIdentity() and got the same result.

pgavlin · 2020-02-03T21:33:11Z

"data source"-esque calls are hanging for me. If I use setMocks like this:

Pretty sure this was due to a bug in my implementation. Can you try again on the latest changes?

Then, using setMocks to mock the output:

The shape of the state that StackReference expects is

{
    name: // stack name
    outputs: // stack outputs
}

If you change your code to the following, it should work:

pulumi.runtime.setMocks({
    newResource: function(type, name, inputs) {
        if (
            type === "pulumi:pulumi:StackReference" &&
            name === "our-org/other-project/other-stack"
        ) {
            return {
                id: inputs["name"],
                state: {
                    name: inputs["name"],
                    outputs: {
                        crossAccountRole: "fake-arn",
                    },
                },
            };
        }
    // ...

dustinfarris · 2020-02-06T23:55:27Z

Pretty sure this was due to a bug in my implementation. Can you try again on the latest changes?

Provider calls are working now!

If you change your code to the following, it should work:

StackReference mocks are working!

Thanks, Pat!

sdk/nodejs/runtime/mocks.ts

lukehoban · 2020-02-14T18:50:18Z

sdk/nodejs/runtime/mocks.ts

+        return "urn:pulumi:" + [getStack(), getProject(), type, name].join("::");
+    }
+
+    public async invoke(req: any, callback: (err: any, innerResponse: any) => void) {


It's weird to have this be async but then return a result via a callback. Can we pick one or the other?

The callback is to conform with the gRPC API shape. Would you suggest that I rewrite this in terms of raw promises?

sdk/nodejs/runtime/settings.ts

lukehoban · 2020-02-14T18:52:17Z

sdk/nodejs/runtime/settings.ts

@@ -64,7 +84,7 @@ export function _setIsDryRun(val: boolean) {
 * and therefore certain output properties will never be resolved.
 */
 export function isDryRun(): boolean {
-    return options.dryRun === true || isTestModeEnabled();
+    return options.dryRun === true;


Will this change break folks who are using the old test mode?

It will, yes--do you have a suggestion for avoiding this?

lukehoban · 2020-02-14T18:54:10Z

sdk/nodejs/runtime/settings.ts

+    };
+
+    monitor = mockMonitor;
+}


Can we add tests that use this?

sdk/python/lib/pulumi/runtime/mocks.py

These changes add support for mocking the resource monitor to the NodeJS and Python SDKs. The proposed mock interface is a simplified version of the standard resource monitor that allows an end-user to replace the usual implementations of ReadResource/RegisterResource and Invoke with their own. This can be used in unit tests to allow for precise control of resource outputs and invoke results.

pgavlin requested review from lukehoban and CyrusNajmabadi January 12, 2020 22:44

pgavlin requested review from mikhailshilkov and EvanBoyle January 12, 2020 22:47

EvanBoyle approved these changes Jan 13, 2020

View reviewed changes

lukehoban mentioned this pull request Jan 13, 2020

Support testing Pulumi programs using tests written in the source language #2287

Closed

mikhailshilkov mentioned this pull request Jan 16, 2020

Unit testing in .NET #3696

Merged

dustinfarris reviewed Jan 17, 2020

View reviewed changes

dustinfarris reviewed Jan 22, 2020

View reviewed changes

sdk/nodejs/runtime/settings.ts Outdated Show resolved Hide resolved

EvanBoyle mentioned this pull request Jan 28, 2020

Go SDK testing approach (user facing) #3824

Closed

dustinfarris reviewed Jan 28, 2020

View reviewed changes

pgavlin force-pushed the pgavlin/mocks branch from 542d422 to 4a8078a Compare January 29, 2020 19:29

pgavlin force-pushed the pgavlin/mocks branch from 4a8078a to e9424f7 Compare February 3, 2020 21:31

dustinfarris approved these changes Feb 6, 2020

View reviewed changes

lukehoban reviewed Feb 14, 2020

View reviewed changes

pgavlin force-pushed the pgavlin/mocks branch from e9424f7 to 081b930 Compare February 25, 2020 19:26

pgavlin added 6 commits February 28, 2020 15:44

Python fixups

e3ced1b

Fix stack/project in Node

7062b54

Fix bugs, clarify types

c82b71b

Comments

294d014

CL

04142be

pgavlin force-pushed the pgavlin/mocks branch from 081b930 to 04142be Compare February 28, 2020 23:45

pgavlin changed the title ~~WIP: mock resource monitor~~ Mock resource monitor Feb 28, 2020

pgavlin merged commit 682dced into master Feb 29, 2020

pulumi-bot deleted the pgavlin/mocks branch February 29, 2020 01:22

joeduffy mentioned this pull request Mar 8, 2020

Python PULUMI_TEST_MODE gives incorrect errors #4039

Closed

This was referenced Mar 17, 2020

Possible mock resource ordering problem #4097

Closed

Mock resource monitor does not support secrets #4145

Closed

Mock resource monitor #3738

Mock resource monitor #3738

Conversation

pgavlin commented Jan 12, 2020

pgavlin commented Jan 12, 2020

pgavlin commented Jan 12, 2020

EvanBoyle left a comment

Choose a reason for hiding this comment

ringods commented Jan 14, 2020

mikhailshilkov commented Jan 14, 2020

mikhailshilkov commented Jan 16, 2020

dustinfarris commented Jan 16, 2020 • edited Loading

dustinfarris commented Jan 16, 2020

EvanBoyle commented Jan 16, 2020

dustinfarris commented Jan 16, 2020

EvanBoyle commented Jan 16, 2020

dustinfarris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukehoban Jan 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukehoban commented Jan 18, 2020

dustinfarris commented Jan 21, 2020

mikhailshilkov commented Jan 22, 2020

dustinfarris commented Jan 22, 2020 • edited Loading

pgavlin commented Jan 22, 2020

mikhailshilkov commented Jan 22, 2020

pgavlin commented Jan 28, 2020

mikhailshilkov commented Jan 28, 2020

Choose a reason for hiding this comment

dustinfarris commented Jan 28, 2020

pgavlin commented Jan 29, 2020

pgavlin commented Jan 29, 2020

dustinfarris commented Jan 29, 2020

dustinfarris commented Jan 30, 2020

dustinfarris commented Jan 30, 2020

pgavlin commented Feb 3, 2020

dustinfarris commented Feb 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustinfarris commented Jan 16, 2020 •

edited

Loading

lukehoban Jan 18, 2020 •

edited

Loading

dustinfarris commented Jan 22, 2020 •

edited

Loading

dustinfarris commented Feb 6, 2020 •

edited

Loading