forge icon indicating copy to clipboard operation
forge copied to clipboard

fix(plugin-webpack): try other logger ports

Open DevanceJ opened this issue 1 year ago • 14 comments

  • [x] I have read the contribution documentation for this project.
  • [x] I agree to follow the code of conduct that this project follows, as appropriate.
  • [x] The changes are appropriately documented (if applicable).
  • [x] The changes have sufficient test coverage (if applicable).
  • [x] 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 avatar Mar 14 '24 13:03 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.

DevanceJ avatar Mar 14 '24 13:03 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.

DevanceJ avatar Mar 15 '24 07:03 DevanceJ

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.

BlackHole1 avatar Mar 15 '24 09:03 BlackHole1

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

DevanceJ avatar Mar 18 '24 11:03 DevanceJ

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

BlackHole1 avatar Mar 21 '24 03:03 BlackHole1

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

DevanceJ avatar Mar 21 '24 03:03 DevanceJ

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

DevanceJ avatar Mar 21 '24 03:03 DevanceJ

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.

BlackHole1 avatar Mar 21 '24 03:03 BlackHole1

Hey, I have fixed the CI errors in the previous merge from main.

DevanceJ avatar Apr 15 '24 21:04 DevanceJ

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

DevanceJ avatar Apr 20 '24 02:04 DevanceJ

Looking into this error.

DevanceJ avatar May 09 '24 09:05 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 avatar May 11 '24 07:05 DevanceJ

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

DevanceJ avatar May 29 '24 08:05 DevanceJ

@erickzhao please review this. Thank you

DevanceJ avatar Jun 07 '24 03:06 DevanceJ