Skip to content
This repository has been archived by the owner on Oct 23, 2024. It is now read-only.

Fix resolvers returning undefined #19

Conversation

alexstrat
Copy link
Contributor

@alexstrat alexstrat commented Feb 4, 2019

Fixes #17

⚠️I added a failing test case in 993a417: however, all tests passed even with the failing test case. But it failed (correctly) if I'd run only that case in particular. => #24

@@ -396,6 +400,22 @@ describe("graphqlObservable", function() {
m.expect(result.pipe(take(1))).toBeObservable(expected);
});

// fixme: without `only` all tests pass 🤔
itMarbles.only("if defined but returns undefined, field is null", function (m) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

⚠️

Copy link

Choose a reason for hiding this comment

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

I sense async induced race conditions. Will check

Copy link
Contributor Author

@alexstrat alexstrat Mar 4, 2019

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After merging #24, this kind of fix might be required: getstation@1034b76

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in aafb8e0

@alexstrat
Copy link
Contributor Author

@DanielMSchmidt do you have the opportunity to look at this please?

@nLight nLight self-assigned this Feb 15, 2019
@nLight
Copy link

nLight commented Feb 15, 2019

Hey @alexstrat Daniel is off for two weeks I will do my best to support the PR :)

@alexstrat
Copy link
Contributor Author

@nLight ah ok, good for him! No worry, it can wait 2 weeks if not your priority!

@alexstrat
Copy link
Contributor Author

@nLight I have been able to pinpoint the issue with the tests and proposed a fix in #25. Do you think it would be possible to review/merge/release this fix (and #23) soon please?

@DanielMSchmidt
Copy link
Contributor

@alexstrat I will take a look at this tomorrow, sorry for the delay 🙇

@DanielMSchmidt
Copy link
Contributor

I had some time and took a look. I removed ever .only and removed two fields from queries in tests and everything was green. But also with the .only everything passed just fine, so I think I don't understand the problem we are facing here 🙈

@alexstrat
Copy link
Contributor Author

alexstrat commented Mar 25, 2019

@DanielMSchmidt, I'm sorry, I don't understand your last comment 😬

I think I don't understand the problem we are facing here

What problem are you refering to? #24 (skipped tests) or #17 (missing field for resolvers returning undefined)

@alexstrat
Copy link
Contributor Author

I rebased with master and made necessary changes in aafb8e0

@DanielMSchmidt DanielMSchmidt merged commit 68334fd into d2iq-archive:master Mar 26, 2019
@mesosphere-frontend-ci
Copy link

🎉 This PR is included in version 3.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

4 participants