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
Merged

Conversation

DxCx
Copy link
Contributor

@DxCx DxCx 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
Copy link
Contributor Author

DxCx 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
Copy link
Contributor

@helfer helfer left a comment

Choose a reason for hiding this comment

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

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 () {
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this change related to this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@DxCx DxCx merged commit fd77345 into apollographql:master Jan 24, 2017
@DxCx DxCx deleted the batching#176 branch January 24, 2017 10:38
@@ -457,6 +469,29 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
});
});

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants