search

broccolijs / broccoli

Browser compilation library – an asset pipeline for applications that run in the browser
https://broccoli.build
MIT License
3.33k stars 216 forks source link

Make tests more resilient (don’t crash if an obviously used port is b… #347

Closed stefanpenner closed 6 years ago

stefanpenner commented 6 years ago

…eing used)

chrmod commented 6 years ago

@stefanpenner I've noticed that if tests fail due to taken port it often means the previous test did not finish/cleanup correctly. It sometimes help to detect a real problem with test test or with the code, so I'm not sure if this change is worth it. Also, there are some cli tests that also start the server, so if you decide to go with this direction it may be worth take a look on other ones. (one way to detect which ones start serve is by looking on console output).

stefanpenner commented 6 years ago

@stefanpenner I've noticed that if tests fail due to taken port it often means the previous test did not finish/cleanup correctly.

This will still happen, as the scenario where this is possible is during the same test run, and we only use 1 PORT per complete test run.

It sometimes help to detect a real problem with test test or with the code, so I'm not sure if this change is worth it.

I believe the only way cross test run leaks can happen is if we use subprocesses for anything, and we do not.

stefanpenner commented 6 years ago

Also, there are some cli tests that also start the server,

Good catch, I will update these.

stefanpenner commented 6 years ago

Also, there are some cli tests that also start the server, Good catch, I will update these.

Actually I do not believe they do. As the mocks don't actually call into the true server stuff. (tested by utilizing the ports used in the CLI tests, and the tests continued to pass correctly)