feat: try other logger ports

[DevanceJ]

• I have read the contribution documentation for this project.
• I agree to follow the code of conduct that this project follows, as appropriate.
• The changes are appropriately documented (if applicable).
• The changes have sufficient test coverage (if applicable).
• The testsuite passes successfully on my local machine (if applicable).

Summarize your changes:
Fixes #3204

1. Checks for availability of ports from 9000-9010.
2. Shows which port has been assigned.
3. Throws an appropriate error message for better user experience.

[DevanceJ]

Currently I am only checking till port 9009, @erickzhao can you guide me if we should increase the upper limit for this port availability checking.

[BlackHole1]

findAvailablePort should be split into two functions for easier unit testing and logical decoupling.
I think it would be better if you can move the port-related functions to a different location, as I remember similar requirements elsewhere in the forge.

Moreover, in my opinion, the Logger doesn't actually need to include port-related code, it would be better if it were passed from the outside. For example: logger.start(port).

It would be great if you could add relevant unit tests. Thank you for your contribution!

[DevanceJ]

Hi @BlackHole1, just made the changes that you suggested in code review and wrote unit tests for Logger.ts. I couldn't directly test the private functions such as portOccupied and findAvailablePort, so I wrote test for start() such that the private functions are tested without changing Access levels.

Thank you for the code review, I hope this fixes the issue. Glad to Contribute.

[BlackHole1]

Can you move portOccupied and findAvailablePort to packages/utils/core-utils/src?

1. Create a file port.ts in the packages/utils/core-utils/src directory.
2. Move portOccupied and findAvailablePort to port.ts as arrow functions (not as class members).
3. Add a file port_spec.ts in the packages/utils/core-utils/test directory and write unit tests.

[DevanceJ]

Note:
Currently one of the tests that I have written are dependent on port 49152 i.e if that port is occupied one my test will fail. We can get rid of this dependency by removing the tests that use 49152 because the tests that check for rejection are indirectly checking for these test.

I am new to writing tests so I might be wrong

[BlackHole1]

LGTM. Thank you for contribute.

PTAL @dsanders11 @erickzhao

[BlackHole1]

When I updated the current branch, the CI failed, it seems to be related to Code Lint. @DevanceJ, can you try to fix it?

[DevanceJ]

Hi Kevin, I just saw this. Working on it now!

[DevanceJ]

Looking at the time the tests take, can you check my pr adding parallelism in circleci/config #3536.😁

[BlackHole1]

Looking at the time the tests take, can you check my pr adding parallelism in circleci/config #3536.😁

I am not very familiar with CircleCI, so #3536 may need to be reviewed by @dsanders11 and @georgexu99.

[DevanceJ]

Hi Erick,
I have Refactored portOccupied function for clarity and reliability. Updated test cases accordingly.

BlackHole1 left a LGTM comment above

[DevanceJ]

Looking into this error.

[DevanceJ]

Hi I am getting this error

TSError: ⨯ Unable to compile TypeScript: packages/plugin/webpack/src/WebpackPlugin.ts:574:42 - error TS2554: Expected 0 arguments, but got 1. 574 this.loggerPort = await logger.start(this.loggerPort as number);

when I run yarn test locally but the start method does expect a parameter in reality. Any help on this is appreciated.

[DevanceJ]

Hi sorry for the delay, I believe there was a naming conflict with the start function.

[DevanceJ]

@erickzhao please review this. Thank you

had 1 more comment

[erickzhao]

We should also update the API comment on this to reflect that we will attempt to start up to port+10 if the port is unavailable.

forge/packages/plugin/webpack/src/Config.ts

Line 154 in f0dd217

* The TCP port for web-multi-logger. Defaults to 9000.

[erickzhao]

We should clean up this.port in the constructor if it isn't being used anymore.