willmorgan / stub-azure-function-context

Create a stub context object for use in testing your Function Apps
BSD 3-Clause "New" or "Revised" License
22 stars 9 forks source link

When binding definitions in a test file will differ from function.json definitions? #11

Closed maximivanov closed 1 year ago

maximivanov commented 3 years ago

First, appreciate the time you guys spent to craft this. Very thorough implementation of Azure protocol.

I'm probably missing something but specifying binding definitions in the test file seems redundant to me. Since I'm testing some function I really want binding definitions to be the same as in the function.json because that's how Azure runtime will invoke my function.

The question is: what's the valid scenario where I may want to override binding definitions configured in function.json? I'm thinking about adding a little helper which would parse and load relevant bits from the config file, so that a user only needs to provide a trigger object.

willmorgan commented 3 years ago

As I recall, the original function.json schema allowed you to define bindings in two ways, with the latter explicit bindings block "inheriting" from the implicit bindings created with triggers.

It's been a while since I've touched this so maybe @dhensby can chime in. He's still typing out Function Apps these days.

dhensby commented 3 years ago

I think it would be fine to load the function.json file, but I'm not sure it should be the default.

I also think it is so straightforward (require('../function.json');) that a helper doesn't seem necessary...

dhensby commented 3 years ago

I do suppose some kind of bootstrapping function would be nice.

const functionHandler = bootstrapFunction(funcUnderTest, 'path/to/function.json', triggers);
it('works', () => {
    const result = await functionHandler();
    expect(result).to.equal(something);
});

In my test suites I usually have a helper function to do this, but it would be a nice little syntactic sugar...

maximivanov commented 3 years ago

How about something like this. I'm still quite new to all this but I guess in 99% cases there will be only one input binding? In case there are multiple, it expects passed triggers to be in the same order as defined in the function.json.

function bootstrapFunction(
  functionUnderTest: AzureFunction,
  functionConfigPath: string,
  triggers: any[],
  date = new Date(),
) {
  return () => {
    let functionConfig

    try {
      functionConfig = require(functionConfigPath)
    } catch (e) {
      // Make error message helpful
      throw new Error(`Couldn't find function config at ${functionConfigPath}`)
    }

    const bindings = functionConfig.bindings
    bindings.forEach(
      (binding: any, index: number) => (binding.data = triggers[index]),
    )

    return runStubFunctionFromBindings(functionUnderTest, bindings, date)
  }
}

And usage is as following:

it('returns user', () => {
  const queryParams = { userId: 123 }
  const handler = bootstrapFunction(functionUnderTest, '../function.json', [
    createHttpTrigger('GET', 'http://example.com', {}, {}, null, queryParams),
  ])

  const res = await handler()
  expect(res.user.id).toEqual(123)
})
dhensby commented 3 years ago

@maximivanov sorry for not responding in a timely manner, I didn't see this notification.

I think what you propose looks really nice. The only question I'd have is how to reliably link the triggers from the function.json with the supplied triggers, especially as triggers can be in or out under "direction"? presumably we wouldn't want to supply output bindings?

maximivanov commented 3 years ago

@dhensby indeed it may be tricky to add such an abstraction utility method usable for all trigger types without making it confusing.

In the end I think it's fine if library consumers define a little helper method in their code (basically what you suggested initially 😀).

Here's some real-world example I ended up with: https://github.com/maximivanov/azure-function-graphql-typescript-starter/blob/main/graphql/__tests__/util/azure.ts#L22

Basically it's just finding a in http trigger (I believe there can be only one) because that's what I'm testing there.

And then it's used like so: https://github.com/maximivanov/azure-function-graphql-typescript-starter/blob/main/graphql/__tests__/integration/PostResolver.test.ts#L9

As an improvement to the docs, maybe the usage example can be updated to require the binding config instead of listing it in-line in the runStubFunctionFromBindings call? I think the fact that binding were passed explicitly confused me in the first place.

dhensby commented 3 years ago

Without being very involved I think the only thing we can do is to offer a thin helper. I do think a helper that allows you to give the named trigger data would be nice, though.

The problem with adding the require call to the docs is that we actually need to change the bindings by adding a data property to them.

It's a bit gross and could definitely be improved as this was really just a first attempt / proof-of-concept of what I wanted to achieve

maximivanov commented 3 years ago

Thinking about it, the bootstrap function code from my comment above can be improved so that it doesn't rely on the trigger/bindings order in the function.json.

Assumptions:

Considering that, the code can expect named data objects for bindings:

function bootstrapFunction(
  functionUnderTest: AzureFunction,
  functionConfigPath: string,
  bindingStubs: any[],
  date = new Date(),
) {
  return () => {
    let functionConfig

    try {
      functionConfig = require(functionConfigPath)
    } catch (e) {
      // Make error message helpful
      throw new Error(`Couldn't load function config at ${functionConfigPath}`)
    }

    const bindings = functionConfig.bindings
    bindings.forEach(
      (binding: any) => {
        if (binding.direction === 'in' && bindingStubs[binding.name]) {
          binding.data = bindingStubs[binding.name]
        }
      }
    )

    return runStubFunctionFromBindings(functionUnderTest, bindings, date)
  }
}

And usage:

it('returns user', () => {
  const queryParams = { userId: 123 }
  const handler = bootstrapFunction(functionUnderTest, '../function.json', {
    // "req" is the name of a corresponding "httpTrigger" trigger defined in function.json
    req: createHttpTrigger('GET', 'http://example.com', {}, {}, null, queryParams),
  })

  const res = await handler()
  expect(res.user.id).toEqual(123)
})
willmorgan commented 3 years ago

Re-familiarising myself with the docs, I'm currently struggling to see how this feature can't achieve what you're trying to do.

Sure, passing in a function.json path would be nice, but that's a solved problem and not hard to do. I'd like to try and keep this library small - and keep the core API developer-friendly.

I can see the utility in some sort of merge operation, like below:

const testReqBinding = {
    req: createHttpTrigger("GET", "http://jobsite.co.uk", {}, {}, null, queryParams)
}
const testBindings = bindingsFrom(
    path.resolve(__dirname, "../function.json"),
    (theParsedBindings) {
        return Object.assign(theParsedBindings, testReqBinding)
    }
)

But then the more I think about use cases, the more complex adding "official" support for this becomes. Some people might wanna use deep merge, or still wanna use lodash, etc, etc. I can see quite a few scenarios where a dev might want to override just a single property of an individual binding?

Also, one usage pattern could even set output bindings to be a stub, mock or spy.

I think on balance, if we can add some function that's open enough and allows varied and real use cases and doesn't try to do too much, that would be good. Probably nicer to refactor/replace runFunctionFromStubBindings if and when it happens.

maximivanov commented 3 years ago

I found this library when I had a very simple need. I wanted to pass a stub request to a http-triggered function and make sure the response is correct. Sure, there may be more advanced use cases (your example with mocking output bindings 👍). But I guess most of the time it's "I want to send this as the input and see what's in the output".

When I read the documentation I was confused, why does it ask me to provide full bindings config? Why do I even need to configure the output binding in my tests? I considered function config as part of the code I'm testing.

Whether it deserves a comment in the docs or a helper function or none, I'm not sure. I just wanted to point out the fact it was confusing when I landed here.

In my future projects, where again I will need to test output for a given input, I will probably use a helper function similar to what I posted above to abstract away bindings configuration.

dhensby commented 1 year ago

this is addressed in v2 where the function.json file can be provided to the runner.