feat: Backend support and tests for refresh runs from previous at version #1320
Conversation
There was a problem hiding this comment.
@stalgiag thanks for doing all this prep work. I'm sure it'll certainly go a long way so as to not have overly huge PRs for this incoming and exciting change!
The direction of things here makes sense to me. I left a question inline though that makes me wonder if I'm testing existing functionality as intended, so would love clarification on that.
| removeAtVersionById, | ||
| findOrCreateAtVersion, | ||
| getUniqueAtVersionsForReport, | ||
| getRefreshableTestPlanReports: getRefreshableTestPlanReportsForVersion |
There was a problem hiding this comment.
nit: Is there a reason for not exporting with the function name. Or should the function name be renamed to getRefreshableTestPlanReports?
Just wondering if it's because of the sole import location and that context why the rename happened here. If so, I personally would rather the rename happen where the function is being imported because this just causes a tiny bit of initial confusion when looking at these exports.
There was a problem hiding this comment.
Ah good callout! No this is just due to me renaming the function using my IDE's "auto-rename" feature and the feature making a choice to keep the name the same in the export. Can fix
There was a problem hiding this comment.
Addressed in 5c01031
| @@ -585,13 +592,6 @@ const scheduleCollectionJob = async ( | |||
| transaction | |||
| }); | |||
|
|
|||
| const atVersion = await getAtVersionWithRequirements( | |||
There was a problem hiding this comment.
Given this removal, doesn't the scheduleCollectionJobResolver#L17 now need to also pass atVersion? Assigning a bot now immediately fails or is that intended as part of the follow-up?
There was a problem hiding this comment.
I didn't finish addressing the changes on the frontend and that is part of why I didn't want to merge this work until more had aggregated here. So, yes, it is meant to be a follow-up but the work shouldn't be merged without it
But, the point you have here raises a really good question that makes me wonder why the scheduleCollectionJob gql mutation in the integration test is still working. Will look into it
There was a problem hiding this comment.
This was still working because of the way that the test version of a collection job is initialized. Luckily no eerie mysteries this time.
I took the resolver to a slightly more logical stopping point for the frontend work with 5c01031
| @@ -27,11 +27,28 @@ BEGIN | |||
| END; | |||
| $$; | |||
|
|
|||
| CREATE OR REPLACE FUNCTION get_at_version_id(atId integer, p_name text) RETURNS integer | |||
…automatable version (#1325) * Working verdict copying * Correct auto finalization behavior on refresh * Also test that report is not finalized when not all output is the same
|
superceded by #1348 |
This pull request adds backend support and testing for the refresh feature.
To keep this in a semi-reviewable state, I am going to open additional PRs targeting this one. This should be reviewed first but I will target this branch with subsequent related work.
Remaining related tasks (follow-up)
Notes
AtServiceintoAtServiceandAtVersionService. Apologies for the noise in the PRCan be reviewed but should not be merged since I want to aggregate additional changes here.