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

Added client testing docs #79

Merged
merged 9 commits into from
Aug 15, 2020
Merged

Conversation

GabrielDzul
Copy link
Contributor

After a couple of days struggling to test the client side with webmock, I decided to share the solution as the original docs lacks of this information.
Hope this helps other users!

Copy link
Collaborator

@dblock dblock left a comment

Choose a reason for hiding this comment

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

I made some suggestions on the README flow.

I also think we should have these examples as working tests in the test suite to ensure that they actually work.

README.md Outdated
@@ -389,6 +389,61 @@ describe App do
end
```

In order to test the client side `stub_request` with Webmock.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Building on the above, I think this part should say ...

In order to stub the response to actual queries, dump the schema into a JSON file and specify it via schema_path as follows.

# your example

Now the first note is taken care by the intro, and the second note is unrelated here - I think if we're going to describe what happens in camel case vs. underscore it should be its own section and we need to show where this happens and why.

Copy link
Owner

Choose a reason for hiding this comment

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

Auto camelCase <-> snake-case happens in graphq-ruby

README.md Outdated
)
end

it 'responds ok when querying data' do
Copy link
Collaborator

Choose a reason for hiding this comment

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

it returns the expected data, not just responds ok

Copy link
Owner

@ashkan18 ashkan18 left a comment

Choose a reason for hiding this comment

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

Awesome! 😍 Thanks for documenting! I'll merge once you addressed @dblock 's comments ❤️

README.md Outdated
@@ -389,6 +389,61 @@ describe App do
end
```

In order to test the client side `stub_request` with Webmock.
Copy link
Owner

Choose a reason for hiding this comment

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

Auto camelCase <-> snake-case happens in graphq-ruby

@GabrielDzul
Copy link
Contributor Author

Updated the docs and added the example as a working test.
Thank you!

require 'spec_helper'

describe 'App' do
include_context 'Dummy Client'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is unnecessary/potentially misleading.

I believe you only need this:

  include Rack::Test::Methods

Check and also match in the docs?


```ruby
describe App do
let(:url) { 'http://graph.biz/graphql' }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you need include Rack::Test::Methods as well.

Copy link
Contributor Author

@GabrielDzul GabrielDzul Aug 15, 2020

Choose a reason for hiding this comment

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

include Rack::Test::Methods is not necessary when using rspec and webmock. That's the reason the test work in the suite. Including it will lead to confusion as users may think that is required in order to work.

@dblock
Copy link
Collaborator

dblock commented Aug 15, 2020

One more fix regarding the test methods, please.

If you feel like it, since this is a schema for an invoice API, maybe rename schema.json into invoice_api.json and things like 'returns expected data' into 'returns invoice fees', just to show intent. I'm really nitpicking :)

@GabrielDzul
Copy link
Contributor Author

Updated PR with requested changes

@dblock dblock merged commit 2214ae4 into ashkan18:master Aug 15, 2020
@dblock
Copy link
Collaborator

dblock commented Aug 15, 2020

Merged, thank you!

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

Successfully merging this pull request may close these issues.

3 participants