Fix jasmine 3.4

[davimacedo]

Trying to help on #5502

[acinader]

nice

[codecov]

Codecov Report

Merging #5573 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #5573      +/-   ##
==========================================
- Coverage   94.06%   94.01%   -0.05%     
==========================================
  Files         129      129              
  Lines        9192     9192              
==========================================
- Hits         8646     8642       -4     
- Misses        546      550       +4

Impacted Files	Coverage Δ
src/ParseServer.js	96.35% <100%> (ø)	⬆️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js	91.68% <0%> (-0.25%)	⬇️
src/Controllers/DatabaseController.js	94.9% <0%> (-0.19%)	⬇️
...dapters/Storage/Postgres/PostgresStorageAdapter.js	96.81% <0%> (-0.17%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9232fcc...d890afe. Read the comment docs.

[davimacedo]

I think it is all passing now. @acinader can you please take a look?

[acinader]

@davimacedo this looks good to me and I'll merge it now.

__indexBuildCompletionCallbackForTests:

1. terrible name!
2. I think we could probably use __indexBuildCompletionCallbackForTests: in ALL the places where we now have a delay in both the helper.js and the specs.
3. these changes are good for catching uncaught promise rejections in the specs, it doesn't help for production run time where the __indexBuildCompletionCallbackForTests is a no-op (though pretty simple to fix I suppose).

Nice.

[davimacedo]

@acinader I can send an additional PR with these:

1. terrible name!

I can change the name to a shorter one. Maybe "dbLoadCallback" ?

2. I think we could probably use __indexBuildCompletionCallbackForTests: in ALL the places where we now have a delay in both the helper.js and the specs.

I can tackle this

3. these changes are good for catching uncaught promise rejections in the specs, it doesn't help for production run time where the __indexBuildCompletionCallbackForTests is a no-op (though pretty simple to fix I suppose).

I am not sure what you mean exactly, but, when changing the name, we can remove the "__" and, instead of returning the promise, we could just call the "dbLoadCallback" when the promise resolves/fails. I think it can be useful in production for some situations.

What do you think?

[acinader]

1. That name is an improvement for sure. I think that serverStartComplete may end up to be more accurate?

2. Overall, helper.js is a real mess, especially the 'after' steps where we iterate, just waiting for the DB to be done....so any improvement there would go a long way to eliminating our flakey tests. I think this is the hard part. It's possible (likely), that I am confusing two things though: a) server startup/db startup and b) hook completion. You can see this: https://community.parseplatform.org/t/async-aftersave-notification-on-completion/65 for my thoughts...

3. What I mean is that as it stands now, the default __indexBuildCompletionCallbackForTests

is a no-op: __indexBuildCompletionCallbackForTests = () => {},

So if one of the promises it is passed is rejected, we'll still have an unhandled rejection.

I suspect that the right thing to do is catch the rejection and then shut the server down in an orderly way with the error message of the rejection.

[acinader]

I just hit another example...I forgot to start mongo before starting my parse server which results in an

UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 3)

[davimacedo]

Got it. I've just sent 3 PRs (still waiting Travis validate the tests) where serverStartComplete < avoidUnhandledPromiseRejection < removeTestDelays, so you can decide which to merge now and perhaps which can be merged later.

"serverStartComplete" just changes the callback name and address - your issue 1;

"avoidUnhandledPromiseRejection" improves the way that server initialization rejections are handled in prod according to your suggestion - your issue 3; I also added one test simulating the problem you had (database not started);

"removeTestDelays" removes some delays from the tests - your issue 2. I analyzed all the tests delay one-by-one and I'd say there are some different groups: a) 1 unique delay related to server initialization (and I removed) b) few delays related to the problem you mentioned in the community forum (and I solved) c) delays that are important for the test (we will not be able to remove them) d) delays that will require us to change the parse server to have additional callbacks/promises (I think those are hard to be addressed and we can work on them as long as we have opportunity) e) Delays that could be removed changing the test, but not so easy (I think we can work on them later as well.

[acinader]

Thanks for the thoughtful analysis and solid solution. I think we should go for 'removeTestDelays' which I just reviewed.