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

Run batching queries in parallel (#176) #273

Merged
merged 6 commits into from Jan 24, 2017

Conversation

@DxCx
Copy link
Member

commented Jan 23, 2017

TODO:

  • Update CHANGELOG.md with your change (include reference to issue & this PR)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

Closes #176

@DxCx

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2017

@helfer Please approve, i'll rebase & merge once the other PRs are merged..

@DxCx DxCx changed the title Batching#176 Run batching queries in parallel (#176) Jan 23, 2017
@DxCx DxCx force-pushed the DxCx:batching#176 branch from 71235a1 to fa5292f Jan 23, 2017
@helfer
helfer approved these changes Jan 23, 2017
Copy link
Member

left a comment

Nice! I especially like how you used a field to test for the delay.

Feel free to merge this if you're happy with it now, and then ping me to release a new version.

@@ -154,7 +154,9 @@ const version = 'modern';
describe(`GraphQL-HTTP (apolloServer) tests for ${version} express`, () => {
describe('POST functionality', () => {

it('allows gzipped POST bodies', async () => {
it('allows gzipped POST bodies', async function () {

This comment has been minimized.

Copy link
@helfer

helfer Jan 23, 2017

Member

How is this change related to this PR?

This comment has been minimized.

Copy link
@DxCx

DxCx Jan 24, 2017

Author Member

Node 4 is much slower then Node 6.
I increase those 2 tests timeout since there is timeout after exhausting node abit..

@DxCx DxCx force-pushed the DxCx:batching#176 branch from 322c00c to 3c249eb Jan 24, 2017
@DxCx DxCx merged commit fd77345 into apollographql:master Jan 24, 2017
3 checks passed
3 checks passed
CLA Author has signed the Meteor CLA.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 98.028%
Details
@DxCx DxCx deleted the DxCx:batching#176 branch Jan 24, 2017
@@ -457,6 +469,29 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
});
});

it('can handle batch requests in parallel', function() {

This comment has been minimized.

Copy link
@glasser

glasser Jan 24, 2017

Contributor

Another potential testing strategy that doesn't require the test to take a long time is to have much shorter timeouts in the queries but to expect the part of one query after a timeout to be able to observe the effect of another query.

This comment has been minimized.

Copy link
@DxCx

DxCx Jan 24, 2017

Author Member

yeah i thought about creating a promise and resolve it on the other query.
but it felt just too bad..
this is probably what i would've done if it was not going through the whole query engine and such.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.