Top-down iterative migration to async #93
Conversation
Co-authored-by: DeeDeeG <DeeDeeG@users.noreply.github.com>
Co-authored-by: DeeDeeG <DeeDeeG@users.noreply.github.com>
|
No surprise it failed when the tests also need to be adapted to the changing call signature... |
|
I can see this is something we both want to happen lol, as I've started something similar over on #88. Otherwise, as you can see the tests fail without any helpful logging at all, the tests being fixed to provide much more useful logging has been added over on #87 so I may recommend grabbing those updates once it's merged, which may make this process easier. |
|
@confused-Techie yes, actually I think the testing is a blocker. I'm not sure if this is even doable with this obsolete "fork of fork of fork" version of Jasmine. For now, I'm going to rebase onto #87 to see if it changes this fundamentally; if not, it might be completely pointless to try. |
with a very tiny amount of code beautification on the way
|
@2colours I actually totally agree that the testing is a bit of a blocker. So by all means hopefully having better tests here can help you to get your PR working. The testing is the main thing that stopped me from continuing my work of async. And while I'm pretty sure everyone agrees the fork of a fork of jasmine is problematic, it's the same testing code used on Pulsar, and in turn what's used within every single community package. So there's lots of momentum propping it up, so we will probably have to work with it for quite some time more. |
|
@confused-Techie Actually I'm not sure there is much sense in using this version of Jasmine only because other packages are using it. Besides, do you think it will ever become easier to move away from it? I think it's plain to see that at some point, this move must happen. Do you think there will be a point where it will suddenly get easier or more reasonable to do it than "as soon as possible"? |
|
@2colours Maybe I should have been slightly more clear. What I really meant to say, is we will likely have to always support this testing framework in Pulsar. As Pulsar is the test framework for every single community package, and if we change it in Pulsar we break every single community package's testing. And to move away here, may only makes things more complex, as we are then using multiple different testing suites between two very closely related codebases. So in all reality, probably improving on the existing testing framework may make more sense, if we have to keep using it in the biggest codebase anyways. But to answer your question, no I don't think it will ever been any easier. The only way it'd become any easier is if we ever manage to fully integrate PPM into Pulsar, at which time this entire codebase would likely be essentially rewritten, which would allow for new tests to be created. But beyond that, it will never be easier than it is now, albeit, likely won't become that much harder. But trust me, I've brought up a few times the possibility of moving testing suites a couple times. And so have other team members. I'd love to do a proper investigation into this, and scope out what our possibilities may be. But I don't see it happening today. If we do want to move away from it, probably would be best to continue this discussion in Discord, and get more opinions on it, and see where other people are at. |
| } else if (message) { | ||
| console.error(message.red); | ||
| } | ||
| return void reject(error); |
There was a problem hiding this comment.
I'm pretty sure this is somewhat equivalent to return undefined. Unless the side effect of running reject(error) is all you care about here -- if the resulting value of reject(error) needs to genuinely be returned, then void should not be used here.
I haven't seen void used in a JS context before, that I can recall, so I had to look it up:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/void
(My thoughts on void in JS: Seems kinda funky, IMO. Dunno when it'd be needed.)
No time for a full-on review at the moment, this just stood out to me and I thought I'd comment on it.
There was a problem hiding this comment.
Yes, the side effect is all I care about here. Basically reject(error); return;, or from a different approach: returning right after a function call while preventing the value to leak out.
|
As it can be seen, testing is really problematic with the change of the "topmost interface". I'm going to try the same thing but omitting this layer of asincification. If that can be tested, I'm going to open a draft PR for that and close this one for history. |
|
Closing this PR as the migration of the main API with this test framework seems like a tedious, if not downright impossible task. |
If I know it right, it's possible for maintainers to edit this PR directly.
My approach is the following: