TestResults module bugfixes#646
Open
vagisha wants to merge 6 commits into
Open
Conversation
…re-chart bugs - TestResultsController.parseDate: parse dates strictly so 13/45/2026 is rejected instead of rolling over to a valid date; give each call its own SimpleDateFormat instead of one shared static one (SimpleDateFormat is not thread-safe). - TrainRunAction / DeleteRunAction: recompute a user's training stats when a run is removed from their training set or deleted, and remove the stats row when no training runs remain (recomputeUserData helper). - DeleteRunAction: delete the run's handleleaks and trainruns rows before deleting the run itself, so deleting a run that had handle leaks or was a training run no longer fails with a foreign-key (FK) error. - failureDetail.jsp: set the chart's date-axis bounds only when dates exist, avoiding a JavaScript error on an empty date range. - TestResultsTest: added a delete-with-children test; tightened the date and training-data tests. Co-Authored-By: Claude <noreply@anthropic.com>
* testRemoveTrainingRunRecomputesStatsWhenRunsRemain verifies that removing one of a user's two training runs keeps the UserData row and recomputes meanmemory from the remaining run (prior tests only hit the delete branch) * Added selectRows/getUserId/runAverageMem/userDataMeanMemory helpers Co-Authored-By: Claude <noreply@anthropic.com>
# Conflicts resolved: # testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java
…ore the recompute on the 'force' path too, so a force request with a bad runId returns "run does not exist" instead of throwing on an empty details[0] * TestResultsTest.testRemoveTrainingRunRecomputesStatsWhenRunsRemain: also assert stddevmemory (|a-b|/2 with two training runs, 0 with one remaining) Co-Authored-By: Claude <noreply@anthropic.com>
* TrainRunAction: check the run exists before recompute on the force path too, so a bad runId returns "run does not exist" instead of throwing (with a test) * testRemoveTrainingRunRecomputesStatsWhenRunsRemain: compare memory stats with an exact epsilon instead of a loose tolerance Co-Authored-By: Claude <noreply@anthropic.com>
* Added getRunInContainer helper; trainRun, deleteRun, flagRun, viewLog, viewXml and the flagged-runs list now reject or omit runs from other folders, since run ids are global and the raw testruns table is not container-filtered (a user could otherwise act on another folder's run by guessing its id) * deleteRun/trainRun now recompute training stats for the verified in-folder run, removing the cross-folder recompute gap * testResultsTest.testRunAccessIsContainerScoped: cross-folder access test Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses multiple correctness and security issues in the testresults module that were discovered during Selenium test development, including (1) stale training statistics, (2) delete failures due to missing child-row cleanup, (3) a failure-chart JS error when no dates are present, (4) missing container scoping for run-by-id actions, and (5) lenient/non-thread-safe date parsing.
Changes:
- Fix training stats recomputation/deletion by centralizing logic in
recomputeUserData(...)and invoking it from both training and run-deletion paths. - Enforce folder/container scoping for run-by-id actions and flagged-run listing to prevent cross-folder run access by guessed IDs.
- Harden UI and parsing: guard failure chart axis bounds when no dates exist; switch to strict, per-call
SimpleDateFormatparsing forMM/dd/yyyy.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java | Adds Selenium/API coverage for strict date validation, child-row-safe deletes, training stats recomputation (update/delete branches), force-path behavior, and cross-folder run access prevention. |
| testresults/src/org/labkey/testresults/view/failureDetail.jsp | Prevents c3/d3 errors by only setting x-axis min/max when the chart has dates to plot. |
| testresults/src/org/labkey/testresults/TestResultsController.java | Implements strict date parsing, container-scoped run lookup, safe child-row deletion ordering, and training-stat recomputation to eliminate stale stats and cross-folder access. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale
Fixes several bugs in the testresults module found while writing Selenium tests, plus a folder-scoping gap in how
runs are looked up by id:
Related Pull Requests
Changes
recomputeUserData(scope, userid, container), which recomputes a user's stats from their remaining training runs and deletes theUserDatarow when none remain. Called byTrainRunActionandDeleteRunAction.DeleteRunAction: delete all child rows (now including handleleaks and trainruns) before the parent run, then refresh the owner's training stats.TrainRunAction: verify the run exists before recompute on the force path too.getRunInContainer(runId, container);trainRun,deleteRun,flagRun,viewLog,viewXmland the flagged-runs list now reject or omit runs from other folders.showRunandTrainingDataViewActionwere already scoped.failureDetail.jsp: set the chart's date-axis bounds only when dates exist.Testing
Added new tests for strict dates, deleting a run with child rows, the recompute update branch (mean and stddev), the trainRun force-path error, and cross-folder run access (testRunAccessIsContainerScoped).
Co-Authored-By: Claude noreply@anthropic.com