Skip to content
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.

Sign up for GitHub

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

Set 250 as an upper limit for retries #27797

Closed
wants to merge 4 commits into from
Closed

Set 250 as an upper limit for retries #27797

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c17e8e6
Set 250 as an upper limit for retries
MuazOthman Sep 12, 2023
832b2fd
Add and update tests for retries upper limit
MuazOthman Sep 12, 2023
ff515d1
refactored to provide more detailed error messages
MuazOthman Sep 15, 2023
558447f
Fixed unit tests
MuazOthman Sep 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 46 additions & 8 deletions packages/config/__snapshots__/validation.spec.ts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,15 +267,15 @@ exports['missing key "flaky"'] = {
'value': {
'default': 1,
},
'type': 'an object with keys `default` and `flaky`. Keys `default` and `flaky` must be integers greater than 0.',
'type': 'an object with keys `default` and `flaky`. Keys `default` and `flaky` must be integers between 0 and 250.',
}

exports['missing key "default"'] = {
'key': 'experimentalBurnIn',
'value': {
'flaky': 1,
},
'type': 'an object with keys `default` and `flaky`. Keys `default` and `flaky` must be integers greater than 0.',
'type': 'an object with keys `default` and `flaky`. Keys `default` and `flaky` must be integers between 0 and 250.',
}

exports['keys cannot be zero key'] = {
Expand All @@ -284,7 +284,7 @@ exports['keys cannot be zero key'] = {
'default': 0,
'flaky': 0,
},
'type': 'an object with keys `default` and `flaky`. Keys `default` and `flaky` must be integers greater than 0.',
'type': 'an object with keys `default` and `flaky`. Keys `default` and `flaky` must be integers between 0 and 250.',
}

exports['keys cannot be zero'] = {
Expand All @@ -293,7 +293,7 @@ exports['keys cannot be zero'] = {
'default': 0,
'flaky': 0,
},
'type': 'an object with keys `default` and `flaky`. Keys `default` and `flaky` must be integers greater than 0.',
'type': 'an object with keys `default` and `flaky`. Keys `default` and `flaky` must be integers between 0 and 250.',
}

exports['keys cannot be negative'] = {
Expand All @@ -302,7 +302,7 @@ exports['keys cannot be negative'] = {
'default': -3,
'flaky': -40,
},
'type': 'an object with keys `default` and `flaky`. Keys `default` and `flaky` must be integers greater than 0.',
'type': 'an object with keys `default` and `flaky`. Keys `default` and `flaky` must be integers between 0 and 250.',
}

exports['keys cannot be floating point numbers'] = {
Expand All @@ -311,7 +311,7 @@ exports['keys cannot be floating point numbers'] = {
'default': 5.7,
'flaky': 8.22,
},
'type': 'an object with keys `default` and `flaky`. Keys `default` and `flaky` must be integers greater than 0.',
'type': 'an object with keys `default` and `flaky`. Keys `default` and `flaky` must be integers between 0 and 250.',
}

exports['keys cannot be Infinity'] = {
Expand All @@ -320,7 +320,7 @@ exports['keys cannot be Infinity'] = {
'default': null,
'flaky': null,
},
'type': 'an object with keys `default` and `flaky`. Keys `default` and `flaky` must be integers greater than 0.',
'type': 'an object with keys `default` and `flaky`. Keys `default` and `flaky` must be integers between 0 and 250.',
}

exports['extraneous keys'] = {
Expand All @@ -330,7 +330,7 @@ exports['extraneous keys'] = {
'flaky': 5,
'notflaky': null,
},
'type': 'an object with keys `default` and `flaky`. Keys `default` and `flaky` must be integers greater than 0.',
'type': 'an object with keys `default` and `flaky`. Keys `default` and `flaky` must be integers between 0 and 250.',
}

exports['config/src/validation .isValidRetriesConfig experimental options fails with invalid strategy 1'] = {
Expand Down Expand Up @@ -663,3 +663,41 @@ exports['config/src/validation .isValidRetriesConfig experimental options fails
},
'type': 'a positive number or null or an object with keys "openMode" and "runMode" with values of numbers, booleans, or nulls, or experimental configuration with key "experimentalStrategy" with value "detect-flake-but-always-fail" or "detect-flake-and-pass-on-threshold" and key "experimentalOptions" to provide a valid configuration for your selected strategy',
}

exports['invalid retry object 2'] = {
'key': 'mockConfigKey',
'value': {
'runMode': 251,
},
'type': 'a positive number or null or an object with keys "openMode" and "runMode" with values of numbers, booleans, or nulls, or experimental configuration with key "experimentalStrategy" with value "detect-flake-but-always-fail" or "detect-flake-and-pass-on-threshold" and key "experimentalOptions" to provide a valid configuration for your selected strategy',
}

exports['invalid retry object 3'] = {
'key': 'mockConfigKey',
'value': {
'openMode': 251,
},
'type': 'a positive number or null or an object with keys "openMode" and "runMode" with values of numbers, booleans, or nulls, or experimental configuration with key "experimentalStrategy" with value "detect-flake-but-always-fail" or "detect-flake-and-pass-on-threshold" and key "experimentalOptions" to provide a valid configuration for your selected strategy',
}

exports['config/src/validation .isValidRetriesConfig experimental options fails with detect-flake-but-always-fail: maxRetries is greater than 250 1'] = {
'key': 'mockConfigKey',
'value': {
'experimentalStrategy': 'detect-flake-but-always-fail',
'experimentalOptions': {
'maxRetries': 251,
},
},
'type': 'a positive number or null or an object with keys "openMode" and "runMode" with values of numbers, booleans, or nulls, or experimental configuration with key "experimentalStrategy" with value "detect-flake-but-always-fail" or "detect-flake-and-pass-on-threshold" and key "experimentalOptions" to provide a valid configuration for your selected strategy',
}

exports['config/src/validation .isValidRetriesConfig experimental options fails with detect-flake-and-pass-on-threshold: maxRetries is greater than 250 1'] = {
'key': 'mockConfigKey',
'value': {
'experimentalStrategy': 'detect-flake-and-pass-on-threshold',
'experimentalOptions': {
'maxRetries': 251,
},
},
'type': 'a positive number or null or an object with keys "openMode" and "runMode" with values of numbers, booleans, or nulls, or experimental configuration with key "experimentalStrategy" with value "detect-flake-but-always-fail" or "detect-flake-and-pass-on-threshold" and key "experimentalOptions" to provide a valid configuration for your selected strategy',
}
10 changes: 5 additions & 5 deletions packages/config/src/validation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ export const isValidBrowserList = (_key: string, browsers: any): ErrResult | tru
const isValidExperimentalRetryOptionsConfig = (options: any, strategy: 'detect-flake-but-always-fail' | 'detect-flake-and-pass-on-threshold'): boolean => {
if (options != null) {
// retries must be an integer of 1 or greater
const isValidMaxRetries = _.isInteger(options.maxRetries) && options.maxRetries > 0
const isValidMaxRetries = _.isInteger(options.maxRetries) && options.maxRetries > 0 && options.maxRetries <= 250

if (!isValidMaxRetries) {
return false
Expand Down Expand Up @@ -163,7 +163,7 @@ export const isValidRetriesConfig = (key: string, value: any): ErrResult | true
const experimentalOptions = ['experimentalStrategy', 'experimentalOptions']
const experimentalStrategyOptions = ['detect-flake-but-always-fail', 'detect-flake-and-pass-on-threshold']

const isValidRetryValue = (val: any) => _.isNull(val) || (Number.isInteger(val) && val >= 0)
const isValidRetryValue = (val: any) => _.isNull(val) || (Number.isInteger(val) && val >= 0 && val <= 250)
const optionalKeysAreValid = (val: any, k: string) => optionalKeys.includes(k) && isValidRetryValue(val)

if (isValidRetryValue(value)) {
Expand Down Expand Up @@ -462,14 +462,14 @@ export function isValidBurnInConfig (key: string, value: any): ErrResult | true
const { default: defaultKey, flaky: flakyKey, ...extraneousKeys } = value

if (defaultKey !== undefined && defaultKey !== undefined && _.isEmpty(extraneousKeys)) {
const isDefaultKeyValid = _.isInteger(defaultKey) && _.inRange(defaultKey, 1, Infinity)
const isFlakyKeyValid = _.isInteger(flakyKey) && _.inRange(flakyKey, 1, Infinity)
const isDefaultKeyValid = _.isInteger(defaultKey) && _.inRange(defaultKey, 1, 250)
const isFlakyKeyValid = _.isInteger(flakyKey) && _.inRange(flakyKey, 1, 250)

if (isDefaultKeyValid && isFlakyKeyValid) {
return true
}
}
}

return errMsg(key, value, 'an object with keys `default` and `flaky`. Keys `default` and `flaky` must be integers greater than 0.')
return errMsg(key, value, 'an object with keys `default` and `flaky`. Keys `default` and `flaky` must be integers between 0 and 250.')
}
38 changes: 33 additions & 5 deletions packages/config/test/validation.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,16 +138,25 @@ describe('config/src/validation', () => {

result = validation.isValidRetriesConfig(mockKey, 2)
expect(result).to.be.true

result = validation.isValidRetriesConfig(mockKey, 250)
expect(result).to.be.true
})

it('returns true for valid retry objects', () => {
let result = validation.isValidRetriesConfig(mockKey, { runMode: 1 })

expect(result).to.be.true

result = validation.isValidRetriesConfig(mockKey, { runMode: 250 })
expect(result).to.be.true

result = validation.isValidRetriesConfig(mockKey, { openMode: 1 })
expect(result).to.be.true

result = validation.isValidRetriesConfig(mockKey, { openMode: 250 })
expect(result).to.be.true

result = validation.isValidRetriesConfig(mockKey, {
runMode: 3,
openMode: 0,
Expand All @@ -165,6 +174,14 @@ describe('config/src/validation', () => {
result = validation.isValidRetriesConfig(mockKey, { fakeMode: 1 })
expect(result).to.not.be.true
snapshot('invalid retry object', result)

result = validation.isValidRetriesConfig(mockKey, { runMode: 251 })
expect(result).to.not.be.true
snapshot('invalid retry object 2', result)

result = validation.isValidRetriesConfig(mockKey, { openMode: 251 })
expect(result).to.not.be.true
snapshot('invalid retry object 3', result)
})

it('returns true for valid retry object with experimental keys (default)', () => {
Expand Down Expand Up @@ -316,6 +333,18 @@ describe('config/src/validation', () => {
snapshot(result)
})

it(`${strategy}: maxRetries is greater than 250`, () => {
const result = validation.isValidRetriesConfig(mockKey, {
experimentalStrategy: strategy,
experimentalOptions: {
maxRetries: 251,
},
})

expect(result).to.not.be.true
snapshot(result)
})

it(`experimentalStrategy is "${strategy}" with only "maxRetries" in "experimentalOptions"`, () => {
const result = validation.isValidRetriesConfig(mockKey, {
experimentalStrategy: strategy,
Expand Down Expand Up @@ -897,14 +926,13 @@ describe('config/src/validation', () => {
expect(validate).to.be.true
})

// TODO: revisit limit on 'default' and 'flaky' keys to set sane limits
it(`tests upper bound (currently no limit and is subject to change)`, () => {
it(`tests upper bound is 250`, () => {
const validate = validation.isValidBurnInConfig('experimentalBurnIn', {
default: 100000,
flaky: 142342342,
default: 251,
flaky: 251,
})

expect(validate).to.be.true
expect(validate).to.not.be.true
})
})
})