Skip to content

TestResults module bugfixes #646

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

Open
vagisha wants to merge 6 commits into
base: release26.3-SNAPSHOT
Choose a base branch
from
+451 −58
Open

TestResults module bugfixes #646

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
07d5822
Fixed testresults date-parsing, training-stats, run-delete, and failu…
vagisha Jun 14, 2026
5e70afe
Added TestResultsTest coverage for the recomputeUserData update branch
vagisha Jun 17, 2026
5daec5e
Merge release26.3-SNAPSHOT into 26.3_fb_testresults-bug-fixes
vagisha Jun 18, 2026
26e984f
* TestResultsController.TrainRunAction: check that the run exists bef…
vagisha Jun 18, 2026
f56aa32
Hardened trainRun force path and tightened recompute test assertions
vagisha Jun 20, 2026
2368193
Scoped testresults run-by-id actions to the current folder
vagisha Jun 21, 2026
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
172 changes: 131 additions & 41 deletions testresults/src/org/labkey/testresults/TestResultsController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,18 @@
public class TestResultsController extends SpringActionController
{
private static final Logger _log = LogManager.getLogger(TestResultsController.class);
private static final SimpleDateFormat MDYFormat = new SimpleDateFormat("MM/dd/yyyy");
private static final String MDY_PATTERN = "MM/dd/yyyy";

// SimpleDateFormat is not thread-safe and is lenient by default, so we never share a
// single static instance. Each caller gets a fresh, strict formatter: strict parsing
// makes nonsense dates like 13/45/2026 throw ParseException instead of silently rolling
// over to a valid-but-wrong date (month 13 -> +1 year, day 45 -> spill into next month).
private static SimpleDateFormat mdyFormat()
{
SimpleDateFormat format = new SimpleDateFormat(MDY_PATTERN);
format.setLenient(false);
return format;
}

private static final String KEY_SUCCESS = "Success";

Expand Down Expand Up @@ -175,7 +186,76 @@ public static String getTabClass(String tabName, String activeTab)

private static Date parseDate(String dateStr) throws ParseException
{
return StringUtils.isNotBlank(dateStr) ? MDYFormat.parse(dateStr) : null;
return StringUtils.isNotBlank(dateStr) ? mdyFormat().parse(dateStr) : null;
}

/**
* Loads the run with the given id only if it belongs to the given container, otherwise returns
* null. Run ids are global and the raw testruns table has no query-layer container filter, so
* actions that look a run up by id must scope by container or a user could read or modify a run
* in another folder by guessing its id.
*/
private static RunDetail getRunInContainer(int runId, Container container)
{
SimpleFilter filter = new SimpleFilter(FieldKey.fromParts("id"), runId);
filter.addCondition(FieldKey.fromParts("container"), container.getEntityId());
return new TableSelector(TestResultsSchema.getTableInfoTestRuns(), filter, null).getObject(RunDetail.class);
}

/**
* Recomputes one user's training statistics (mean/stddev of tests-run and memory) in the
* given container from their remaining training runs. If the user has no training runs
* left, deletes their UserData row instead.
*
* The delete branch matters: the recompute groups by (userid, container), so an empty
* join produces zero output rows, ON CONFLICT never fires, and a stale UserData row would
* otherwise survive with its old mean/stddev values (leaving the user listed on the
* training-data page as if they still had training data). Callers must invoke this inside
* an open transaction.
*/
private static void recomputeUserData(DbScope scope, int userid, Container container)
{
SQLFragment remainingFragment = new SQLFragment();
remainingFragment.append(
"SELECT COUNT(*) FROM " + TestResultsSchema.getTableInfoTestRuns() + " " +
"JOIN " + TestResultsSchema.getTableInfoTrain() +
" ON " + TestResultsSchema.getTableInfoTestRuns() + ".id = " + TestResultsSchema.getTableInfoTrain() + ".runid " +
"WHERE userid = ? AND container = ?");
remainingFragment.add(userid);
remainingFragment.add(container.getEntityId());
long remainingTrainRuns = new SqlSelector(TestResultsSchema.getSchema(), remainingFragment).getObject(Long.class);

if (remainingTrainRuns == 0)
{
SQLFragment deleteFragment = new SQLFragment();
deleteFragment.append("DELETE FROM " + TestResultsSchema.getTableInfoUserData() + " WHERE userid = ? AND container = ?");
deleteFragment.add(userid);
deleteFragment.add(container.getEntityId());
new SqlExecutor(scope).execute(deleteFragment);
}
else
{
SQLFragment sqlFragmentUpdate = new SQLFragment();
sqlFragmentUpdate.append(
"INSERT INTO " + TestResultsSchema.getTableInfoUserData() + " " +
" (userid, container, meantestsrun, meanmemory, stddevtestsrun, stddevmemory) " +
"SELECT ?, ?, avg(passedtests), avg(averagemem), stddev_pop(passedtests), stddev_pop(averagemem) " +
"FROM " + TestResultsSchema.getTableInfoTestRuns() + " " +
"JOIN " + TestResultsSchema.getTableInfoTrain() +
" ON " + TestResultsSchema.getTableInfoTestRuns() + ".id = " + TestResultsSchema.getTableInfoTrain() + ".runid " +
"WHERE userid = ? AND container = ? " +
"GROUP BY userid, container " +
"ON CONFLICT(userid, container) DO UPDATE SET " +
" meantestsrun = excluded.meantestsrun, " +
" meanmemory = excluded.meanmemory, " +
" stddevtestsrun = excluded.stddevtestsrun, " +
" stddevmemory = excluded.stddevmemory");
sqlFragmentUpdate.add(userid);
sqlFragmentUpdate.add(container.getEntityId());
sqlFragmentUpdate.add(userid);
sqlFragmentUpdate.add(container.getEntityId());
new SqlExecutor(scope).execute(sqlFragmentUpdate);
}
}

// Form class for RetrainAllAction
Expand Down Expand Up @@ -529,16 +609,14 @@ public Object execute(TrainRunForm form, BindException errors)
SqlSelector sqlSelector = new SqlSelector(TestResultsSchema.getSchema(), sqlFragment);
List<Integer> foundRuns = new ArrayList<>();
sqlSelector.forEach(rs -> foundRuns.add(rs.getInt("runid")));
SimpleFilter filter = new SimpleFilter();
filter.addCondition(FieldKey.fromParts("id"), runId);
RunDetail[] details = new TableSelector(TestResultsSchema.getTableInfoTestRuns(), filter, null).getArray(RunDetail.class);
if (!force)
{
if (details.length == 0)
return new ApiSimpleResponse(Map.of(KEY_SUCCESS, false, "error", "run does not exist: " + runId));
else if ((train && !foundRuns.isEmpty()) || (!train && foundRuns.isEmpty()))
return new ApiSimpleResponse(Map.of(KEY_SUCCESS, false, "error", "no action necessary"));
}
// The run must exist (in this folder) even on the force path: recomputeUserData below
// uses run.getUserid(), so a missing run would otherwise throw. Scoping by container
// also stops a run in another folder from being trained from here.
RunDetail run = getRunInContainer(runId, getContainer());
if (run == null)
return new ApiSimpleResponse(Map.of(KEY_SUCCESS, false, "error", "run does not exist: " + runId));
if (!force && ((train && !foundRuns.isEmpty()) || (!train && foundRuns.isEmpty())))
return new ApiSimpleResponse(Map.of(KEY_SUCCESS, false, "error", "no action necessary"));
DbScope scope = TestResultsSchema.getSchema().getScope();
try (DbScope.Transaction transaction = scope.ensureTransaction())
{
Expand All @@ -551,27 +629,8 @@ else if ((train && !foundRuns.isEmpty()) || (!train && foundRuns.isEmpty()))
fragment.add(runId);
new SqlExecutor(scope).execute(fragment);
}
// update user table calculations
SQLFragment sqlFragmentUpdate = new SQLFragment();
sqlFragmentUpdate.append(
"INSERT INTO " + TestResultsSchema.getTableInfoUserData() + " " +
" (userid, container, meantestsrun, meanmemory, stddevtestsrun, stddevmemory) " +
"SELECT ?, ?, avg(passedtests), avg(averagemem), stddev_pop(passedtests), stddev_pop(averagemem) " +
"FROM " + TestResultsSchema.getTableInfoTestRuns() + " " +
"JOIN " + TestResultsSchema.getTableInfoTrain() +
" ON " + TestResultsSchema.getTableInfoTestRuns() + ".id = " + TestResultsSchema.getTableInfoTrain() + ".runid " +
"WHERE userid = ? AND container = ? " +
"GROUP BY userid, container " +
"ON CONFLICT(userid, container) DO UPDATE SET " +
" meantestsrun = excluded.meantestsrun, " +
" meanmemory = excluded.meanmemory, " +
" stddevtestsrun = excluded.stddevtestsrun, " +
" stddevmemory = excluded.stddevmemory");
sqlFragmentUpdate.add(details[0].getUserid());
sqlFragmentUpdate.add(getContainer().getEntityId());
sqlFragmentUpdate.add(details[0].getUserid());
sqlFragmentUpdate.add(getContainer().getEntityId());
new SqlExecutor(scope).execute(sqlFragmentUpdate);
// Refresh this user's training statistics from their remaining training runs.
recomputeUserData(scope, run.getUserid(), getContainer());
transaction.commit();
}
return new ApiSimpleResponse(KEY_SUCCESS, true);
Expand Down Expand Up @@ -990,14 +1049,39 @@ public Object execute(RunIdForm form, BindException errors)
}

int rowId = form.getRunId();
// Look up the run (scoped to this folder) before deleting, both to reject a run in
// another folder and to refresh the owner's training stats afterward: the run may
// have been part of their training set, so their baseline can change (or disappear)
// once it is gone.
RunDetail run = getRunInContainer(rowId, getContainer());
if (run == null)
{
response.put(KEY_SUCCESS, false);
response.put("error", "run does not exist: " + rowId);
return response;
}
// Child tables keyed by testrunid -> testruns(id).
SimpleFilter filter = new SimpleFilter();
filter.addCondition(FieldKey.fromParts("testrunid"), rowId);
try (DbScope.Transaction transaction = TestResultsSchema.getSchema().getScope().ensureTransaction()) {
// trainruns keys the run via its "runid" column, not "testrunid".
SimpleFilter trainFilter = new SimpleFilter();
trainFilter.addCondition(FieldKey.fromParts("runid"), rowId);
DbScope scope = TestResultsSchema.getSchema().getScope();
try (DbScope.Transaction transaction = scope.ensureTransaction()) {
// Delete every child row that references this run before the parent, or the
// testruns delete fails on a foreign key constraint. handleleaks and trainruns
// were previously missed: deleting a run that had handle-leak records or was in
// the training set failed (e.g. fk_memoryleaks_testruns on table handleleaks).
Table.delete(TestResultsSchema.getTableInfoTestFails(), filter);
Table.delete(TestResultsSchema.getTableInfoTestPasses(), filter);
Table.delete(TestResultsSchema.getTableInfoMemoryLeaks(), filter);
Table.delete(TestResultsSchema.getTableInfoHandleLeaks(), filter);
Table.delete(TestResultsSchema.getTableInfoHangs(), filter);
Table.delete(TestResultsSchema.getTableInfoTrain(), trainFilter);
Table.delete(TestResultsSchema.getTableInfoTestRuns(), rowId); // delete run last because of foreign key
// Refresh the owner's training stats now that the run (and any trainruns row for
// it) is gone, so a removed training run does not leave a stale UserData row behind.
recomputeUserData(scope, run.getUserid(), getContainer());
transaction.commit();
} catch (Exception x) {
response.put(KEY_SUCCESS, false);
Expand Down Expand Up @@ -1039,10 +1123,9 @@ public Object execute(FlagRunForm form, BindException errors)
int rowId = form.getRunId();
boolean flag = form.getFlag() != null ? form.getFlag() : false;

SimpleFilter filter = new SimpleFilter();
filter.addCondition(FieldKey.fromParts("id"), rowId);
try (DbScope.Transaction transaction = TestResultsSchema.getSchema().getScope().ensureTransaction()) {
RunDetail detail = new TableSelector(TestResultsSchema.getTableInfoTestRuns(), filter, null).getObject(RunDetail.class);
// Scoped to this folder so a run in another folder cannot be flagged from here.
RunDetail detail = getRunInContainer(rowId, getContainer());
if (detail == null)
{
response.put(KEY_SUCCESS, false);
Expand Down Expand Up @@ -1096,8 +1179,11 @@ public static class ShowFlaggedAction extends SimpleViewAction<Object>
@Override
public ModelAndView getView(Object o, BindException errors) throws Exception
{
// Scoped to this folder so the Flags page lists only flagged runs in the current folder,
// not flagged runs across every folder.
SimpleFilter filter = new SimpleFilter();
filter.addCondition(FieldKey.fromParts("flagged"), true);
filter.addCondition(FieldKey.fromParts("container"), getContainer().getEntityId());
RunDetail[] details = new TableSelector(TestResultsSchema.getTableInfoTestRuns(), filter, null).getArray(RunDetail.class);
JspView<TestsDataBean> view = new JspView<>("/org/labkey/testresults/view/flagged.jsp", new TestsDataBean(details, new User[0]));
view.setFrame(WebPartView.FrameType.PORTAL);
Expand Down Expand Up @@ -1196,9 +1282,11 @@ public Object execute(RunIdForm form, BindException errors)
if (form.getRunId() == null)
return new ApiSimpleResponse("log", null);
int runId = form.getRunId();
// Scoped to this folder so a run's log in another folder cannot be read by id.
SQLFragment sqlFragment = new SQLFragment();
sqlFragment.append("SELECT log FROM testresults.testruns WHERE id = ?");
sqlFragment.append("SELECT log FROM testresults.testruns WHERE id = ? AND container = ?");
sqlFragment.add(runId);
sqlFragment.add(getContainer().getEntityId());
List<byte[]> logs = new ArrayList<>();
SqlSelector sqlSelector = new SqlSelector(TestResultsSchema.getSchema(), sqlFragment);
sqlSelector.forEach(rs -> logs.add(rs.getBytes("log")));
Expand All @@ -1217,9 +1305,11 @@ public Object execute(RunIdForm form, BindException errors)
if (form.getRunId() == null)
return new ApiSimpleResponse("xml", null);
int runId = form.getRunId();
// Scoped to this folder so a run's XML in another folder cannot be read by id.
SQLFragment sqlFragment = new SQLFragment();
sqlFragment.append("SELECT xml FROM testresults.testruns WHERE id = ?");
sqlFragment.append("SELECT xml FROM testresults.testruns WHERE id = ? AND container = ?");
sqlFragment.add(runId);
sqlFragment.add(getContainer().getEntityId());
List<byte[]> xmls = new ArrayList<>();
SqlSelector sqlSelector = new SqlSelector(TestResultsSchema.getSchema(), sqlFragment);
sqlSelector.forEach(rs -> xmls.add(rs.getBytes("xml")));
Expand Down Expand Up @@ -1963,7 +2053,7 @@ private static void ParseAndStoreXML(String xml, Container c) throws Exception
timestampDay = addDays(timestampDay, 1);
}
lastHour = hour;
String originalDate = MDYFormat.format(timestampDay);
String originalDate = mdyFormat().format(timestampDay);
try {
timestamp = new SimpleDateFormat("MM/dd/yyyy HH:mm").parse(originalDate + " " + ts);
} catch (IllegalArgumentException e) {
Expand Down Expand Up @@ -2015,7 +2105,7 @@ private static void ParseAndStoreXML(String xml, Container c) throws Exception
timestampDay = addDays(timestampDay, 1);
}
lastHour = hour;
String originalDate = MDYFormat.format(timestampDay);
String originalDate = mdyFormat().format(timestampDay);
try {
timestamp = new SimpleDateFormat("MM/dd/yyyy HH:mm").parse(originalDate + " " + ts);
} catch (IllegalArgumentException e) {
Expand Down
21 changes: 14 additions & 7 deletions testresults/src/org/labkey/testresults/view/failureDetail.jsp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,19 @@ $(document).ready(function() {
const problemData = <%=json(problemData, 0)%>;

// Generate date chart.
// Only set explicit timeseries min/max when there are dates to plot. With an empty
// dates array, dates[0] and dates[length - 1] are undefined and c3/d3 throws while
// building the x-axis scale (e.g. when the active date range excludes all sample runs).
const chartDates = problemData.graphData.dates;
const xAxis = {
type: 'timeseries',
localtime: false,
tick: { fit: true, format: '%m/%d' }
};
if (chartDates.length > 0) {
xAxis.min = chartDates[0];
xAxis.max = chartDates[chartDates.length - 1];
}
let dateChart = c3.generate({
bindto: '#failGraph',
data: {
Expand All @@ -277,13 +290,7 @@ $(document).ready(function() {
bar: { width: { ratio: 0.3 } },
subchart: { show: false, size: { height: 20 } },
axis: {
x: {
min: problemData.graphData.dates[0],
max: problemData.graphData.dates[problemData.graphData.dates.length - 1],
type: 'timeseries',
localtime: false,
tick: { fit: true, format: '%m/%d' }
},
x: xAxis,
// y: { tick: { values: %=graphYTicks.getJavaScriptFragment(0)% } }
}
});
Expand Down
Loading