-
Notifications
You must be signed in to change notification settings - Fork 0
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
Generate Scenarios for isolation testing of rules. #38
Conversation
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.
Looks great! Not gonna pretend I understood the underworking of every function but I get the basic idea and it runs real slick. I've noted a few minor possible adjustments
// UTF- 8 encoding with BOM | ||
const bom = '\uFEFF'; | ||
const utf8FileContent = bom + fileContent; | ||
res.setHeader('Content-Type', 'text/csv; charset=utf-8'); | ||
res.setHeader('Content-Disposition', `attachment; filename=${filepath.replace(/\.json$/, '.csv')}`); | ||
res.status(HttpStatus.OK).send(utf8FileContent); |
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.
This is almost the same code as in two other places in this file, so you could throw it in a function
const decisionResult = await this.decisionsService.runDecision( | ||
ruleContent, | ||
filepath, | ||
formattedVariablesObject, | ||
{ | ||
trace: true, | ||
}, | ||
); |
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.
Probably outside the scope of this, but this has me thinking we may eventually want to update decisionsService to run decisions on bulk (like many different contexts for the same ruleContent). However, you should update this to make use of Promise.all so that it doesn't have to wait for each decision before running the next one.
|
||
const combinations = this.generateCombinations(ruleSchema, simulationContext, testScenarioCount).slice( | ||
0, | ||
testScenarioCount || 100, |
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.
Better to have testScenarioCount: number = 100,
in the parameters
const ruleRunResults: RuleRunResults = await this.generateTestScenarios( | ||
filepath, | ||
ruleContent, | ||
simulationContext, | ||
testScenarioCount, | ||
); | ||
|
||
const keys = { | ||
inputs: extractUniqueKeys(ruleRunResults, 'inputs'), | ||
expectedResults: extractUniqueKeys(ruleRunResults, 'expectedResults'), | ||
result: extractUniqueKeys(ruleRunResults, 'result'), | ||
}; | ||
|
||
const headers = [ | ||
'Scenario', | ||
'Results Match Expected (Pass/Fail)', | ||
...this.prefixKeys(keys.inputs, 'Input'), | ||
...this.prefixKeys(keys.expectedResults, 'Expected Result'), | ||
...this.prefixKeys(keys.result, 'Result'), | ||
]; | ||
|
||
const rows = Object.entries(ruleRunResults).map(([scenarioName, data]) => [ | ||
this.escapeCSVField(scenarioName), | ||
`n/a`, | ||
...this.mapFields(data.inputs, keys.inputs), | ||
...this.mapFields(data.expectedResults, keys.expectedResults), | ||
...this.mapFields(data.result, keys.result), | ||
]); | ||
|
||
return [headers, ...rows].map((row) => row.join(',')).join('\n'); |
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.
Can we wrap this whole thing in a try/catch, make sure we're throwing errors in the above functions it uses, and then throw an error here too. This way we could return the proper error to the user in scenarioData.controller
if we wanted.
src/utils/csv.ts
Outdated
* @param arrays The arrays to generate the product from. | ||
* @returns The generated product. | ||
*/ | ||
export const cartesianProduct = (arrays: any[][]): any[][] => { |
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.
Should be able to use a generic type "" instead of "any". <T>(arrays: T[][]): T[][]
src/utils/csv.ts
Outdated
* @param limit The maximum number of combinations to generate. | ||
* @returns The generated product. | ||
*/ | ||
export const complexCartesianProduct = (arrays: any[][], limit: number = 3000): any[][] => { |
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.
Should be able to use a generic type "" instead of "any".
export const complexCartesianProduct = <T>(arrays: T[][], limit: number = 3000): T[][] => {
const result: T[][] = [];
Implementation of this functionality required the creation of sub functions to: