Update MetricsDetails to add Save button to save both Test and Baseline Tables#1178
Conversation
✅ Deploy Preview for eclipsefdn-adoptium-trss canceled.
|
Added Save Button to Save both TEST and BASELINE statistics
|
dennycjacob
changed the title
-Related:https://github.ibm.com/runtimes/automation/issues/875 -Signed-off-by:Denny Chacko Jacob
dennycjacob
force-pushed
the
save_updated_statistics
branch
from
bcb53e3 to
c3373a6
Compare
|
@annaibm @smlambert - Please Take a look |
|
Thanks @dennycjacob! I will set up the environment locally to test this out and will share my feedback soon. |
|
|
||
| return ( | ||
| <div> | ||
| {/* One Save Button for bothH tables */} |
There was a problem hiding this comment.
Fix typo in comment: bothH → both
| baselineDisabledIterations | ||
| } = req.body; | ||
|
|
||
| console.log('Received update request:', { |
There was a problem hiding this comment.
Should we remove the console.log before merging this into production?
| Annotate bad runs for both test and baseline, then SAVE | ||
| </div> | ||
| </div> | ||
| <Button |
There was a problem hiding this comment.
Consider adding a Tooltip to the Save button to explain why it is disabled. For example, show "Waiting for data to load..." when testStats or baselineStats is null.
| @@ -0,0 +1,132 @@ | |||
| const { TestResultsDB } = require('../Database'); | |||
| const ObjectID = require('mongodb').ObjectID; | |||
There was a problem hiding this comment.
ObjectID is deprecated , use ObjectId from mongodb
| testDisabledIterations, | ||
| baselineDisabledIterations | ||
| }); | ||
|
|
There was a problem hiding this comment.
Consider adding validation for testId, baselineId and benchmarkName before the try block. If any of these required fields are missing, the current code will throw a MongoDB internal error which is hard to debug. An explicit validation check would return a clear error message.
| } | ||
| export default MetricsDetails; | ||
|
|
||
| export default MetricsDetails; No newline at end of file |
| ...metric, | ||
|
|
||
| // original rawValues | ||
| rawValues: metric.rawValues, |
There was a problem hiding this comment.
Redundant rawValues and statValues fields in metrics already.?
| const item = doc.aggregateInfo[key]; | ||
| if (item.benchmarkName === benchmarkName && | ||
| item.buildName && | ||
| item.buildName.includes(buildType)) { |
There was a problem hiding this comment.
buildName.includes(buildType) could produce false positives if any build name contains test or baseline as a substring, for example "build-protest-aarch64" would accidentally match test. Consider using a regex?
| error: error.message | ||
| }); | ||
| } | ||
| }; No newline at end of file |
|
|
||
| if (!updated) { | ||
| console.error(`Benchmark ${benchmarkName} with type ${buildType} not found`); | ||
| return { success: false, error: `Benchmark not found` }; |
There was a problem hiding this comment.
Misleading error message Benchmark not found when build type doesn't match.Consider separating the two checks with specific error messages for each case.Example:
if (!benchmarkFound) {
return {
success: false,
error: `Benchmark "${benchmarkName}" not found`
};
}
if (!updated) {
return {
success: false,
error: `Benchmark "${benchmarkName}" found but build type "${buildType}" did not match`
};
}
|
Apologies @annaibm - I merged this ahead of seeing your review comments. Will need a follow-up PR to apply the changes you have suggested. |
|
No worries! I think review submit button for me didn't go properly. Created a follow-up issue to track the comments in review: https://github.ibm.com/runtimes/automation/issues/884 |
3 participants
Adding Save Button on MetricsDetails Page
Related:https://github.ibm.com/runtimes/automation/issues/875
Signed-off-by:Denny Chacko Jacob