Skip to content

Track Down Unhandled rejections #5502

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

Closed
wants to merge 2 commits into from

Conversation

acinader
Copy link
Contributor

@acinader acinader commented Apr 13, 2019

fixes: #5500 see #5496, #5488

Add @stage88 0eff786 to fix for spied on MongoStorageAdapter.connect() failing

let's see what else fails.

@acinader
Copy link
Contributor Author

So here's what I think is going on....

The jasmine upgrade from 3.2 - 3.3 introduces this change:

Report unhandled rejections as globalErrors.

Merges #1521 from @johnjbarton

This is a good thing that will help us prepare for:

(node:7741) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: woops
(node:7741) DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code. */

@stage88 figured out a solution for one set of failing tests with 0eff786

I am now struggling with this test:

it('fails if database is unreachable', done => {
reconfigureServer({
databaseAdapter: new MongoStorageAdapter({
uri: 'mongodb://fake:fake@localhost:43605/drew3',
}),
}).catch(() => {
//Need to use rest api because saving via JS SDK results in fail() not getting called
request({
method: 'POST',
url: 'http://localhost:8378/1/classes/NewClass',
headers: {
'X-Parse-Application-Id': 'test',
'X-Parse-REST-API-Key': 'rest',
},
body: {},
}).then(fail, response => {
expect(response.status).toEqual(500);
const body = response.data;
expect(body.code).toEqual(1);
expect(body.message).toEqual('Internal server error.');
reconfigureServer().then(done, done);
});
});
});

Which is failing with this uncaught rejection:

Error: connect ECONNREFUSED 127.0.0.1:43605

I'm having trouble figuring out where this uncaught rejection is occuring....

@acinader acinader added type:bug Impaired feature or lacking behavior that is likely assumed help wanted labels Apr 14, 2019
@dplewis
Copy link
Member

dplewis commented Apr 14, 2019

#5500 (comment) Check out my comment

@codecov
Copy link

codecov bot commented Apr 14, 2019

Codecov Report

Merging #5502 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5502      +/-   ##
==========================================
- Coverage   94.04%   93.99%   -0.05%     
==========================================
  Files         125      125              
  Lines        9097     9097              
==========================================
- Hits         8555     8551       -4     
- Misses        542      546       +4
Impacted Files Coverage Δ
src/Adapters/Auth/httpsRequest.js 95.23% <0%> (-4.77%) ⬇️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 91.97% <0%> (-0.25%) ⬇️
...dapters/Storage/Postgres/PostgresStorageAdapter.js 97.01% <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 019cf0a...5549dd7. Read the comment docs.

@acinader
Copy link
Contributor Author

failing node 11:

  - Unhandled promise rejection: Error: connect ECONNREFUSED 127.0.0.1:43605
  - Failed: There were open connections to the server left after the test finished

@stage88
Copy link
Contributor

stage88 commented Apr 14, 2019

I should be able to have another look at this test tonight (Sydney, AU - time).

Sorry if i understood it incorrectly, I think that error - Unhandled promise rejection: Error: connect ECONNREFUSED 127.0.0.1:43605 is expected in that test given there is nothing on uri: 'mongodb://fake:fake@localhost:43605/drew3'.

I can update the test instead to try request.catch instead of request.then...

@acinader
Copy link
Contributor Author

@stage88 I rolled back #5506 jasmine upgrade and merged #5496

I think that there is a promise reject leak or two preventing upgrade for jasmine and node 11.

If you can help track those down, would be great. Thanks for the winston upgrade. so Helpful!

@acinader acinader changed the title Conform tests to account for Jasmine 3.4 Track Down Unhandled rejections Apr 14, 2019
@stage88
Copy link
Contributor

stage88 commented Apr 14, 2019

No worries :) I'll have another go at that test tonight.

@stage88
Copy link
Contributor

stage88 commented Apr 15, 2019

Captured stack trace of where the error originates:

image

@stage88
Copy link
Contributor

stage88 commented Apr 15, 2019

@acinader check out this commit stage88@f131f26

All tests are passing, except Parse.Query Aggregate testing cannot group by date field (excluding createdAt and updatedAt).

@acinader
Copy link
Contributor Author

Nice. Much improved. There are a handful of tests failing:

https://travis-ci.org/parse-community/parse-server/jobs/520004000

fails with:

  1. ParseServerRESTController should handle a get request with full serverURL mount path
  • Unhandled promise rejection: ParseError: 107 schema class name does not revalidate

https://travis-ci.org/parse-community/parse-server/jobs/520004001

fails with:

  1. Server Url Checks handleShutdown, close connection
  • Unhandled promise rejection: Error: Connection pool of the database object has been destroyed.
  • Uncaught exception: TypeError: message.split is not a function
  1. Postgres database init options should fail to create server if schema databaseOptions does not exist
  • Unhandled promise rejection: error: no schema has been selected to create in (line 487)

https://travis-ci.org/parse-community/parse-server/jobs/520004002

fails with:

  1. SchemaController will resolve class creation races appropriately
  • Unhandled promise rejection: ParseError: 103 Class NewClass already exists.

More broadly, our tests are pretty flakey:

  1. https://github.com/parse-community/parse-server/blob/master/spec/helper.js is hard to follow and could use some comments, possibly switch to async / await in a few places to aid readability?

  2. we use setTimeout ALOT to wait random amounts of time in hopes that async acitivites complete, which is both flakey and makes the tests take much longer than they would if we knew when async activity completed.

I attempted to get some conversation going on the topic here:

https://community.parseplatform.org/t/async-aftersave-notification-on-completion/65

On the tests for my professional projects, I have gotten rid of all the setTimeouts by exposing an event emitter that fires when after save hooks complete.

Its rudimentrary for now, but it allows my cloud code to run without blocking clients.

I am not sure what 'un observed' async code is running in all the places we use setTimeout in our tests, but trying to incrementally pick them off by adding an observable/event on completion could help to get our tests more reliable and faster.

And while I am dreaming, I think that one of the biggest services we could do to the parse commun would be to help make testing easy and well understood for the users of parse, so a blog post or two about how we ended making the tests faster and more reliable....

@stage88
Copy link
Contributor

stage88 commented Apr 15, 2019

Ah I see, there are are more :) happy to help when I can.

With the commit I referenced above, I am really not across everything in Parse, I feel uneasy making changes knowing it could negatively impact a lot of Parse users. I would love to spend some more time reviewing that code myself, or maybe get a bit of guidance from maintainers.

Also I agree, there is work that could be done on improving tests, I think many of these chores could be opened up to community, seeking help through "help wanted", etc

@davimacedo davimacedo mentioned this pull request May 9, 2019
@dplewis
Copy link
Member

dplewis commented May 9, 2019

Closing via #5573

@dplewis dplewis closed this May 9, 2019
@mtrezza mtrezza removed the type:bug Impaired feature or lacking behavior that is likely assumed label Jul 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Master tests failing. Need better notification
4 participants