Skip to content
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

Adds mockAsyncRedwoodDirective to test asynchronous Validator or Transformer RedwoodDirectives #5822

Merged
merged 11 commits into from
Jul 4, 2022

Conversation

dthyresson
Copy link
Contributor

Fixes #5713

mockRedwoodDirective does not await the inner validator function, thus making it hard to test that an async validator functions throws an error

  • Adds mockAsyncRedwoodDirective
  • Adds docs with example

@netlify
Copy link

netlify bot commented Jun 24, 2022

Deploy Preview for redwoodjs-docs ready!

Name Link
🔨 Latest commit e94ab1a
🔍 Latest deploy log https://app.netlify.com/sites/redwoodjs-docs/deploys/62c2d03daf3c000009c95cee
😎 Deploy Preview https://deploy-preview-5822--redwoodjs-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@dthyresson dthyresson added topic/graphql release:feature This PR introduces a new feature release:docs This PR only updates docs labels Jun 24, 2022
@dthyresson
Copy link
Contributor Author

@xmonkee Could you take a look at this implementation and the doc examples and confirm that this resolved the issue your raised in #5713 . Thanks!

@dthyresson dthyresson removed the release:docs This PR only updates docs label Jun 24, 2022
@dthyresson dthyresson marked this pull request as ready for review June 26, 2022 01:45
@dthyresson dthyresson requested a review from jtoar June 26, 2022 01:45
Copy link
Contributor

@jtoar jtoar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dthyresson, looking good! Works locally for me. I made a few edits to the docs. The example in the JSDoc may need updating. And I'm a little confused if it should be mockedResolvedValue or resolvedValue for transformer directives?

While this work as is, I have an API suggestion. What if instead of making another function, we just check if directive.onResolverCalled is async or not? That way users don't have to learn another API.

To be more explicit, the code would look like...

export const mockRedwoodDirective: DirectiveMocker = (
  directive,
  executionMock
) => {
  const { directiveArgs = {}, context, ...others } = executionMock

  if (context) {
    setContext(context || {})
  }
  
  // Here's the new code; just do a check here, instead of making another function
  if (directive.onResolverCalled.constructor.name === 'AsyncFunction') {
    return async () => {
      if (directive.type === DirectiveType.TRANSFORMER) {
        const { mockedResolvedValue } = others as TransformerMock
        return await directive.onResolverCalled({
          resolvedValue: mockedResolvedValue,
          context: globalContext,
          ...others,
        } as DirectiveParams)
      } else {
        await directive.onResolverCalled({
          context: globalContext,
          directiveArgs,
          ...others,
        } as DirectiveParams)
      }
    }
  }

  return () => {
    if (directive.type === DirectiveType.TRANSFORMER) {
      const { mockedResolvedValue } = others as TransformerMock
      return directive.onResolverCalled({
        resolvedValue: mockedResolvedValue,
        context: globalContext,
        ...others,
      } as DirectiveParams)
    } else {
      directive.onResolverCalled({
        context: globalContext,
        directiveArgs,
        ...others,
      } as DirectiveParams)
    }
  }
}

I tested it out locally and it seems to work. Thoughts on this approach?

packages/testing/src/api/directive.ts Outdated Show resolved Hide resolved
docs/docs/directives.md Outdated Show resolved Hide resolved
packages/testing/src/api/directive.ts Outdated Show resolved Hide resolved
packages/testing/src/api/directive.ts Outdated Show resolved Hide resolved
* @example
* const mockExecution = mockAsyncRedwoodDirective(myTransformer, {
* context: currentUser,
* resolvedValue: 'Original Value',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs say mockedResolvedValue but here says resolvedValue—which should it be?

packages/testing/src/api/directive.ts Outdated Show resolved Hide resolved
@dthyresson
Copy link
Contributor Author

Thanks @dthyresson, looking good! Works locally for me. I made a few edits to the docs. The example in the JSDoc may need updating. And I'm a little confused if it should be mockedResolvedValue or resolvedValue for transformer directives?

While this work as is, I have an API suggestion. What if instead of making another function, we just check if directive.onResolverCalled is async or not? That way users don't have to learn another API.

To be more explicit, the code would look like...

export const mockRedwoodDirective: DirectiveMocker = (
  directive,
  executionMock
) => {
  const { directiveArgs = {}, context, ...others } = executionMock

  if (context) {
    setContext(context || {})
  }
  
  // Here's the new code; just do a check here, instead of making another function
  if (directive.onResolverCalled.constructor.name === 'AsyncFunction') {
    return async () => {
      if (directive.type === DirectiveType.TRANSFORMER) {
        const { mockedResolvedValue } = others as TransformerMock
        return await directive.onResolverCalled({
          resolvedValue: mockedResolvedValue,
          context: globalContext,
          ...others,
        } as DirectiveParams)
      } else {
        await directive.onResolverCalled({
          context: globalContext,
          directiveArgs,
          ...others,
        } as DirectiveParams)
      }
    }
  }

  return () => {
    if (directive.type === DirectiveType.TRANSFORMER) {
      const { mockedResolvedValue } = others as TransformerMock
      return directive.onResolverCalled({
        resolvedValue: mockedResolvedValue,
        context: globalContext,
        ...others,
      } as DirectiveParams)
    } else {
      directive.onResolverCalled({
        context: globalContext,
        directiveArgs,
        ...others,
      } as DirectiveParams)
    }
  }
}

I tested it out locally and it seems to work. Thoughts on this approach?

@jtoar I like that much better. I was taking my cue from @xmonkee and #5713.

But a single function simplifies much!

@dthyresson
Copy link
Contributor Author

@jtoar Do you want to push that refactoring into this PR? Much appreciated!

@jtoar
Copy link
Contributor

jtoar commented Jun 27, 2022

@dthyresson just pushed the suggestion. This one seems good to go but maybe we can give xmonkee a few days for feedback.

@redwoodjs redwoodjs deleted a comment from Tobbe Jun 29, 2022
@redwoodjs redwoodjs deleted a comment from Tobbe Jun 29, 2022
@redwoodjs redwoodjs deleted a comment from jtoar Jun 29, 2022
@jtoar
Copy link
Contributor

jtoar commented Jun 30, 2022

@dthyresson I've been waiting for xmonkee's feedback here before merging but let me know if you got feedback on another channel.

@dthyresson dthyresson enabled auto-merge (squash) July 4, 2022 10:47
@dthyresson dthyresson merged commit 56d6688 into redwoodjs:main Jul 4, 2022
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Jul 4, 2022
@jtoar jtoar modified the milestones: next-release, v2.1.0 Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature This PR introduces a new feature topic/graphql
Projects
Status: Archived
Development

Successfully merging this pull request may close these issues.

mockRedwoodDirective fails when ValidatorDirective is async
2 participants