Skip to content

Add support for update events #33

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

Merged
merged 3 commits into from
Jan 12, 2021

Conversation

VFabricio
Copy link
Contributor

As discussed on #32, this allows users to listen and respond to HTTP upgrades.

@thomashoneyman
Copy link
Contributor

@VFabricio It looks like the tests are timing out -- can we make sure that they have a set termination time (ideally not too long, to keep the tests speedy)? The code looks good to me, but I'd like to review the tests one more time.

@VFabricio
Copy link
Contributor Author

@thomashoneyman Thank you for looking at this. The tests were timing out because they spin up servers which are kept running even after all assertions are made. The simplest solution I could think of was to start all the tests and exit the process successfully after 10 seconds. This meant bringing in aff and node-process as devDependencies.

This is not very elegant, because an abnormally long running test might not finish before the whole thing gets killed. If you can think of a better way of handling test termination, I can implement it.

@hdgarrood
Copy link
Member

Do the tests time out on master already, or was this an issue introduced by this PR? I don't think the solution of just exiting after a certain amount of time is workable, for the reason you pointed out; it's quite possible that we might see bugs from callbacks not being called, and this approach would mask those failures.

@VFabricio
Copy link
Contributor Author

Yes, they are timing out on master already. The problem is that each test spins up a server, sends a request to it, but never tears it down. So, if the test is successful the process doesn't crash but neither does it exit normally. It just keeps running, as the server is still listening.

The way the tests are currently structured make it awkward to tear down the servers at the end, unless there is a simple way to do that which I'm failing to see. I think what we really want do when the testing the server part of the API is to create the server, send a request to it, assert that the response is the expected one (right now the responses are merely printed out to be checked visually, it seems) and then close the server.

Is it OK if I rewrite the tests in this way?

@hdgarrood
Copy link
Member

If the tests were failing for this reason on master already, I think we should back out the timeout thing and just merge the update stuff now since that looks like it's ready. We can deal with getting the tests working reliably in a separate PR.

@VFabricio VFabricio force-pushed the feat/addUpdateEvent branch from 8b92c90 to 39a66c0 Compare January 12, 2021 16:03
@VFabricio
Copy link
Contributor Author

Makes sense to me. I have reverted the timeout commit.

@thomashoneyman
Copy link
Contributor

Sorry, @VFabricio, I didn't realize the tests were already timing out -- we've just switched over to new CI which actually runs these tests, vs. the previous setup which didn't actually execute them.

@thomashoneyman thomashoneyman merged commit 48458d4 into purescript-node:master Jan 12, 2021
@VFabricio
Copy link
Contributor Author

Don't worry, @thomashoneyman. I could've been clearer and explained that the tests were already timing out.

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.

3 participants