-
Notifications
You must be signed in to change notification settings - Fork 41
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
Update to jasmine 3.4 & fix resulting spec failures #80
Conversation
expect(Q.isPromise(superspawn.spawn(LS))).toBe(true); | ||
expect(Q.isPromise(superspawn.spawn('invalid_command'))).toBe(true); | ||
it('should resolve on success', () => { | ||
return expectAsync(superspawn.spawn(LS)).toBeResolved(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly off-topic: I think it should be possible to use a very dumb command such as echo
that is exactly the same between Windows cmd, ps, and *nix *sh (including Windows "nix" *sh). This should probably be done in a separate PR, someday. Not a blocker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice. Although I hope we will sunset superspawn
before that 😉
@brodybits thanks for the fast response. Just waiting for AppVeyor CI to succeed. |
Yeah, AppVeyor now seems to be slow on Apache Cordova. I generally setup Travis CI & AppVeyor CI on my forks whenever possible, for another sanity check. And +1 for sunsetting In case of improvements that are very closely related, I would personally try to propose them together in a single PR, to be merged in a merge commit, for the sake of clarity. (Also avoid the chance of others doing parallel investigations) Just an idea. I wish GitHub would have some better support for related PRs. Also for people to suggest changes to multiple lines. |
@raphinesse I am wondering if the fix you applied for the failing spec can also be applied in iOS. I pined a few repo's Jasmine dependency to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM
@erisu yeah, I think I see the issue there. It's basically unhandled promises 95% of the time. The rest are oddities related to the combination of Jasmine and Q. Will try to post a fix the following days. |
Replaces #74 together with #79.