Testing: use a better way to get a port to the test app

[arunoda]

Use http module's server.listen() to bind to a random available port.

[rauchg]

Still not very robust. Why not ephemeral ports given by listen() ?

[sedubois]

Random behavior in tests is nice but can also lead to randomly failing tests sometimes hard to track down (saw that in my previous company). How about testing several startups using a predefined list of ports?

[rauchg]

Can you elaborate? How are ephemeral ports error pone?

[arunoda]

Random behavior in tests is nice but can also lead to randomly failing tests sometimes hard to track down

This is not true. We use portfinder and it'll give an available port. It starts with 8000.
Here we've randomized that.

[arunoda]

Still not very robust. Why not ephemeral ports given by listen() ?

That's a good idea. I'll update.

[arunoda]

@rauchg updated as you suggested.

[rauchg]

when you remove a project, you need to re-instantiate the lock. we should be seeing red here, for portmapper

[arunoda]

Yep yep. Need to do it. I use both npm and yarn at the same time. That's a bad habit.

Will update.

[arunoda]

@rauchg done.

[sedubois]

Can you elaborate?

I was just speaking in a general context. A test which is based on random behavior isn't repeatable. It might start to fail sometimes but not every time, might be quite unsettling. A test (explicitly or not) has 3 parts: the "given", the "when" and the "then". It would be quite helpful to know the "given" just by reading the source code.

[rauchg]

But the randomness of the port has no impact in the outcome, correct? Specially considering that ephemeral ports (via listen()) always give us a free port.

[sedubois]

If the randomness has no impact on the outcome, why do you use it? Why make this PR in that case?

[rauchg]

Because let's say you have something running on port 8000, the tests would have failed for the wrong reason

[sedubois]

I just hadn't understood why that port would be randomly unavailable in the first place. Normally the point of a test env is to offer a reproducible context.

Anyways can just discard my comments, it's just that the subject of randomness triggered some sensitive memories from a past life at F-Secure 😄

[sedubois]

I hope the port isn't randomly unavailable in the test automation env? So it's on some developers' laptops that the issue arises? Then other option than randomness might be to allow the dev to configure the port when running tests (with a sensible default).

@rauchg as you said elsewhere, explicit is better than implicit, when possible.

[arunoda]

I think here we started to randomize the base port. But now we are doing something else.
We let node's http module to pick an available port and use for the test app.

[sedubois]

Other issue with random port is that it might hide the fact that another instance of the tests might have been stuck and left running elsewhere on the machine.

[arunoda]

@sedubois we always run test in parallel and Jest know how to report them well.

it might hide the fact that another instance of the tests might have been stuck and left running elsewhere on the machine.

Let's ignore this case for now. Let's do things one by one.
We can create a new issue and continue.

This is a good improvement compared with the current setup as we longer using portfinder.

[sedubois]

👍

[rauchg]

I just hadn't understood why that port would be randomly unavailable in the first place. Normally the point of a test env is to offer a reproducible context.

@sedubois sure but we also run tests upon prepush, prepublish, so the tests have to be useful on developer machines

[rauchg]

Thanks for your input @sedubois, always much appreciated

[timneutkens]

@arunoda updated the branch arunoda#1.

[timneutkens]

Looks good 👍 @arunoda ready for merge?

[arunoda]

I think this is good to merge.

[timneutkens]

Awesome ❤️

[nkzawa]

Please don't remove react and react-dom. Tests fail when you did clean install.

[timneutkens]

Ah right, my fault when resolving merge conflicts.