core(budget): throw error on duplicate budget types #8915
Conversation
patrickhulce
left a comment
There was a problem hiding this comment.
change itself LGTM!
lighthouse-core/config/budget.js
Outdated
| if (isArrayOfUnknownObjects(timings)) { | ||
| budget.timings = timings.map(Budget.validateTimingBudget); | ||
| Budget.assertNoDuplicateStrings(budget.timings.map(r => r.metric), | ||
| `budget[${index}].timings`); |
There was a problem hiding this comment.
should these read budgets[${index}] since that's the property name?
There was a problem hiding this comment.
should these read budgets[${index}] since that's the property name?
yeah, good call
| it('throws when a duplicate resourceType is specified in resourceCounts', () => { | ||
| budget[0].resourceCounts.push({resourceType: 'third-party', budget: 100}); | ||
| assert.throws(_ => Budget.initializeBudget(budget), | ||
| /budget\[0\]\.resourceCounts has duplicate entry of type 'third-party'/); |
There was a problem hiding this comment.
maybe consider expect().toThrow('the message') to avoid regex escape noise when we're doing exact string matching?
There was a problem hiding this comment.
maybe consider expect().toThrow('the message') to avoid regex escape noise when we're doing exact string matching?
yeah, this whole file should probably switch over...
lighthouse-core/config/budget.js
Outdated
| * @param {Array<string>} arr | ||
| * @param {string} arrayName | ||
| */ | ||
| static assertNoDuplicateStrings(arr, arrayName) { |
There was a problem hiding this comment.
arr feels a bit of an unnecessary abbreviation 😛
There was a problem hiding this comment.
arrfeels a bit of an unnecessary abbreviation 😛
very fair :)
something missed in #8727