Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions TestResultSummaryService/routes/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,5 +67,6 @@ app.post('/getSpecificData', require('./getSpecificData'));
app.post('/upsertBuildList', require('./upsertBuildList'));
app.post('/postTapFiles', require('./postTapFiles'));
app.post('/postIssueFeedback', require('./postIssueFeedback'));
app.put('/updateStats', require('./updateStats'));

module.exports = app;
132 changes: 132 additions & 0 deletions TestResultSummaryService/routes/updateStats.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
const { TestResultsDB } = require('../Database');
const ObjectID = require('mongodb').ObjectID;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ObjectID is deprecated , use ObjectId from mongodb


module.exports = async (req, res) => {
const db = new TestResultsDB();

const {
testId,
baselineId,
benchmarkName,
testStats,
baselineStats,
testDisabledIterations,
baselineDisabledIterations
} = req.body;

console.log('Received update request:', {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove the console.log before merging this into production?

testId,
baselineId,
benchmarkName,
testDisabledIterations,
baselineDisabledIterations
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

try {
// Helper function to update a document
const updateDocument = async (docId, stats, disabledIterations, buildType) => {
// Find the document
const doc = await db.findOne({ _id: new ObjectID(docId) });

if (!doc) {
console.error(`Document not found: ${docId}`);
return { success: false, error: `Document ${docId} not found` };
}

if (!doc.aggregateInfo) {
console.error(`No aggregateInfo in document: ${docId}`);
return { success: false, error: `No aggregateInfo in document` };
}

// Find and update the matching aggregateInfo entry
let updated = false;
for (const key in doc.aggregateInfo) {
const item = doc.aggregateInfo[key];
if (item.benchmarkName === benchmarkName &&
item.buildName &&
item.buildName.includes(buildType)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


if (item.metrics) {
// Update each metric in this aggregateInfo
item.metrics = item.metrics.map(metric => {
return {
...metric,

// original rawValues
rawValues: metric.rawValues,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant rawValues and statValues fields in metrics already.?


//original statValues
statValues: metric.statValues,

// new field - filteredStatValues
filteredStatValues: {
mean: stats.mean,
max: stats.max,
min: stats.min,
median: stats.median,
std: stats.std,
CI: stats.CI
},

// disablediterations
disabledIterations: disabledIterations || []
};
});

updated = true;
}
}
}

if (!updated) {
console.error(`Benchmark ${benchmarkName} with type ${buildType} not found`);
return { success: false, error: `Benchmark not found` };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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` 
    };
}

}

// Update the document in database
await db.update(
{ _id: new ObjectID(docId) },
{ $set: { aggregateInfo: doc.aggregateInfo } }
);

console.log(`Successfully updated document: ${docId}`);
return { success: true };
};

// Update test document
const testResult = await updateDocument(
testId,
testStats,
testDisabledIterations,
'test'
);

if (!testResult.success) {
return res.status(400).json(testResult);
}

// Update baseline document
const baselineResult = await updateDocument(
baselineId,
baselineStats,
baselineDisabledIterations,
'baseline'
);

if (!baselineResult.success) {
return res.status(400).json(baselineResult);
}

res.json({
success: true,
message: 'Statistics updated successfully'
});

} catch (error) {
console.error('Error updating stats:', error);
res.status(500).json({
success: false,
error: error.message
});
}
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a newline

107 changes: 105 additions & 2 deletions test-result-summary-client/src/TrafficLight/MetricsDetails.jsx
Original file line number Diff line number Diff line change
@@ -1,23 +1,126 @@
import { Divider } from 'antd';
import React, { useState } from 'react';
import { Divider, Button, message } from 'antd';
import MetricsTable from './MetricsTable';
import { getParams } from '../utils/query';

function MetricsDetails() {
const { testId, baselineId, benchmarkName } = getParams(location.search);

// State to track data from both tables
const [testData, setTestData] = useState(null);
const [baselineData, setBaselineData] = useState(null);
const [testStats, setTestStats] = useState(null);
const [baselineStats, setBaselineStats] = useState(null);
const [saving, setSaving] = useState(false);

// Save handler for both tables
const handleSave = async () => {
if (!testData || !baselineData || !testStats || !baselineStats) {
message.warning('Please wait for data to load');
return;
}

setSaving(true);

try {
// Calculate which iterations are disabled for TEST
const testDisabledIterations = testData
.filter(item => !item.enabled)
.map(item => item.iteration);

// Calculate which iterations are disabled for BASELINE
const baselineDisabledIterations = baselineData
.filter(item => !item.enabled)
.map(item => item.iteration);

console.log('Saving BOTH test and baseline:', {
testDisabledIterations,
baselineDisabledIterations,
testStats,
baselineStats
});

// One API call saves both TEST and BASELINE stats together
const response = await fetch('/api/updateStats', {
method: 'PUT',
headers: {
'Content-Type': 'application/json'
},
body: JSON.stringify({
testId,
baselineId,
benchmarkName,
testStats,
baselineStats,
testDisabledIterations,
baselineDisabledIterations,
})
});

const result = await response.json();

if (result.success) {
message.success('✓ Both TEST and BASELINE statistics saved successfully!');
} else {
message.error('Failed to save: ' + (result.error || 'Unknown error'));
}
} catch (error) {
message.error('Error saving: ' + error.message);
console.error('Save error:', error);
} finally {
setSaving(false);
}
};

return (
<div>
{/* One Save Button for bothH tables */}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix typo in comment: bothHboth

<div style={{
marginBottom: 16,
padding: 16,
background: '#f5f5f5',
borderRadius: 8,
display: 'flex',
justifyContent: 'space-between',
alignItems: 'center'
}}>
<div>
<div style={{ margin: 0, fontSize: 20, fontWeight: 'bold' }}>
Metrics Details - {benchmarkName}
</div>
<div style={{ margin: 0, fontSize: 12, color: 'rgba(0, 0, 0, 1)' }}>
Annotate bad runs for both test and baseline, then SAVE
</div>
</div>
<Button
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

type="primary"
size="large"
onClick={handleSave}
loading={saving}
disabled={!testStats || !baselineStats}
>
Save Both Statistics
</Button>
</div>


<MetricsTable
type="test"
id={testId}
benchmarkName={benchmarkName}
onDataChange={setTestData}
onStatsChange={setTestStats}
/>
<Divider />
<MetricsTable
type="baseline"
id={baselineId}
benchmarkName={benchmarkName}
onDataChange={setBaselineData}
onStatsChange={setBaselineStats}
/>
</div>
);
}
export default MetricsDetails;

export default MetricsDetails;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a newline here

Loading