-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added tests for options #2409
base: main
Are you sure you want to change the base?
Added tests for options #2409
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
import os from 'os'; | ||
import { | ||
computeConcurrencyOption, | ||
validateCompareOptions, | ||
} from '../utils/options'; | ||
|
||
// Mocking process.exit and process.stderr.write for testing | ||
const exitSpy = jest.spyOn(process, 'exit').mockImplementation(() => { | ||
throw new Error('Exit called'); | ||
}); | ||
const stderrSpy = jest.spyOn(process.stderr, 'write').mockImplementation(); | ||
|
||
describe('validateCompareOptions', () => { | ||
afterEach(() => { | ||
exitSpy.mockClear(); | ||
stderrSpy.mockClear(); | ||
}); | ||
|
||
it('should exit process when compareBranch is defined and changed is false', () => { | ||
expect(() => validateCompareOptions('branch', false)).toThrow( | ||
'Exit called', | ||
); | ||
expect(stderrSpy).toHaveBeenCalledWith( | ||
"Option --compareBranch doesn't make sense without option --changed\n", | ||
); | ||
}); | ||
|
||
it('should not exit process when compareBranch is undefined', () => { | ||
expect(() => validateCompareOptions(undefined, false)).not.toThrow(); | ||
expect(exitSpy).not.toHaveBeenCalled(); | ||
}); | ||
}); | ||
|
||
describe('computeConcurrencyOption', () => { | ||
afterEach(() => { | ||
exitSpy.mockClear(); | ||
stderrSpy.mockClear(); | ||
}); | ||
|
||
it('should return the number of CPUs when concurrencyLevel is undefined', () => { | ||
const cpus = os.cpus().length; | ||
const result = computeConcurrencyOption(undefined); | ||
expect(result).toBe(cpus || 1); | ||
}); | ||
|
||
it('should exit process when concurrencyLevel is not a valid number', () => { | ||
expect(() => computeConcurrencyOption('invalid')).toThrow('Exit called'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
expect(stderrSpy).toHaveBeenCalledWith( | ||
'--currencyLevel must be a number greater or equal than 0. You specified "invalid" instead.', | ||
); | ||
}); | ||
|
||
it('should return parsed number when concurrencyLevel is a valid number', () => { | ||
const result = computeConcurrencyOption('2'); | ||
expect(result).toBe(2); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this test is so simple, it's hard to avoid reimplementing the function. I think you have 2 options:
os.cpus().length || 1
in this variable so as to mimic the intended implementation as closely as possible (breaking this up increases the chance of bugs creeping in).os.cpus()
to return different values, e.g. test for2
andundefined
to cover more possibilities.