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

Allow setting current resource in tests #149

Merged

Conversation

00dav00
Copy link
Contributor

@00dav00 00dav00 commented Feb 7, 2021

Allows setting current_resource on specs using with graphql schema test shared context

Closes #138

controller = context[:controller]
resource_names = Array(context[:resource_name])

if context[:current_resource].blank?
Copy link
Member

Choose a reason for hiding this comment

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

Why not the shorter version?

context[:current_resource] ||= resource_names.find do |resource_name|

Copy link
Member

@mcelicalderon mcelicalderon left a comment

Choose a reason for hiding this comment

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

Looks good, just a small suggestion and don't forget to remove commented code.

@mcelicalderon mcelicalderon added the enhancement New feature or request label Feb 8, 2021
@00dav00 00dav00 merged commit ef45044 into master Feb 11, 2021
@00dav00 00dav00 deleted the allow_setting_resources_directly_into_schema_during_tests branch February 11, 2021 12:11
@keithmgould
Copy link

This is super cool -- I see that its closed now but is there a way for new users of the gem to know about how to properly test? Maybe a readme section could be added show the methods included in this PR, or at least a link from readme to the above?

@00dav00
Copy link
Contributor Author

00dav00 commented Feb 11, 2021

Hi @keithmgould , I think that is a great suggestion!
Would you like to create a PR with that addition to the README?

@mcelicalderon
Copy link
Member

I can also do that if no one has done it until I get some time. Important thing to note here is that this is a quick fix that works for all but one scenario, passing no current_resource for an authenticated query. This will still throw an error unless you stub the controller injected in the context. A more complete fix is still on the works as it might require a bigger refactor of how we set the current user in the context and also the removal of queries and mutations that return http redirects in favor of simpler confirmation and password reset flows. This way we might be able not to require the controller to be injected all together

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Testing Authenticated Elements
3 participants