Added tests for options #2409
Added tests for options #2409
Conversation
|
| }); | ||
|
|
||
| it('should return the number of CPUs when concurrencyLevel is undefined', () => { | ||
| const cpus = os.cpus().length; |
There was a problem hiding this comment.
As this test is so simple, it's hard to avoid reimplementing the function. I think you have 2 options:
- Include
os.cpus().length || 1in this variable so as to mimic the intended implementation as closely as possible (breaking this up increases the chance of bugs creeping in). - Mock
os.cpus()to return different values, e.g. test for2andundefinedto cover more possibilities.
| }); | ||
|
|
||
| it('should exit process when concurrencyLevel is not a valid number', () => { | ||
| expect(() => computeConcurrencyOption('invalid')).toThrow('Exit called'); |
There was a problem hiding this comment.
We're actually missing a bug that exists in the implementation here.
It's important to test the boundary cases; in this case, there is a bug that needs fixing: the function will return 0 when '0' is passed when it actually should exit. This needs fixing and we need a test case to cover that. Note that the comment on line 37 of options.ts is also misleading and needs to be updated.
The tests cover different scenarios such as when the compareBranch option is defined and changed is false, when the concurrencyLevel is undefined, when the concurrencyLevel is not a valid number, and when the concurrencyLevel is a valid number. By testing these scenarios, the code can be ensured to work as expected and avoid errors when used in a larger system.
trying to understand the codebase and added new test cased, and learning how to contribute to the open source community as a junior developer. Please let me know if there are any changes needed.