Skip to content
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

Tests which have been re-ran to update results to avoid conflicts between testers still incorrectly display conflicting results #685

Merged
merged 10 commits into from
Jul 19, 2023
Merged

Tests which have been re-ran to update results to avoid conflicts between testers still incorrectly display conflicting results #685

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
45f1bc6
Work on fixes
Paul-Clue May 16, 2023
8a88b33
Fix re-ran tests conflicts
Paul-Clue Jun 6, 2023
db0e822
Fix re-ran tests conflicts
Paul-Clue Jun 6, 2023
7efe8ab
Change loop conditional
Paul-Clue Jun 7, 2023
4a15c7b
Revise updateTestPlanRun blocks
howard-e Jun 7, 2023
c8f1832
commit changes
Paul-Clue Jun 7, 2023
30f1ca0
Fix destructure error
Paul-Clue Jun 7, 2023
2ac4625
Add context object as param
Paul-Clue Jun 8, 2023
a5b72a9
Make suggested changes from code review
Paul-Clue Jul 19, 2023
3c81c6c
Add curly braces to if-block
Paul-Clue Jul 19, 2023
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
163 changes: 163 additions & 0 deletions server/migrations/20230523163856-remove-OtherUnexpectedBehaviorText.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
'use strict';
const populateData = require('../services/PopulatedData/populateData');
const { updateTestPlanRun } = require('../models/services/TestPlanRunService');
const {
updateTestPlanReport
} = require('../models/services/TestPlanReportService');
const conflictsResolver = require('../resolvers/TestPlanReport/conflictsResolver');
const BrowserLoader = require('../models/loaders/BrowserLoader');
const AtLoader = require('../models/loaders/AtLoader');

module.exports = {
up: queryInterface => {
return queryInterface.sequelize.transaction(async transaction => {
const testPlanRunQuery = await queryInterface.sequelize.query(
`SELECT id, "testResults" FROM "TestPlanRun"`,
{
transaction
}
);

const testPlanReportQuery = await queryInterface.sequelize.query(
`SELECT id, status FROM "TestPlanReport"`,
{
transaction
}
);

const atLoader = AtLoader();
const browserLoader = BrowserLoader();
const testPlanRunData = testPlanRunQuery[0];
const testPlanReportsData = testPlanReportQuery[0];
if (!testPlanRunData) {
// eslint-disable-next-line no-console
console.info('The test Results are empty');
return;
}

for (let i = 0; i < testPlanRunData.length; i += 1) {
const testPlanRun = testPlanRunData[i];
const testPlanRunId = testPlanRun.id;
let needsUpdate = false;

let updateParams = {
testResults: testPlanRun.testResults
};

if (!testPlanRunData[i].testResults) {
continue;
}
if (
Array.isArray(testPlanRunData[i].testResults) &&
testPlanRunData[i].testResults.length < 1
) {
continue;
}

for (
let j = 0;
j < testPlanRunData[i].testResults.length;
j += 1
) {
if (!testPlanRunData[i].testResults[j].scenarioResults) {
continue;
}
if (
Array.isArray(
testPlanRunData[i].testResults[j].scenarioResults
) &&
testPlanRunData[i].testResults[j].scenarioResults
.length < 1
) {
continue;
}
for (
let p = 0;
p <
testPlanRunData[i].testResults[j].scenarioResults
.length;
p += 1
) {
if (
!testPlanRunData[i].testResults[j].scenarioResults[
p
].unexpectedBehaviors
) {
continue;
}

if (
Array.isArray(
testPlanRunData[i].testResults[j]
.scenarioResults[p].unexpectedBehaviors
) &&
testPlanRunData[i].testResults[j].scenarioResults[p]
.unexpectedBehaviors.length < 1
) {
continue;
}
for (
let s = 0;
s <
testPlanRunData[i].testResults[j].scenarioResults[p]
.unexpectedBehaviors.length;
s += 1
) {
const unexpectedBehavior =
testPlanRunData[i].testResults[j]
.scenarioResults[p].unexpectedBehaviors[s];
if (
unexpectedBehavior.id !== 'OTHER' &&
unexpectedBehavior.otherUnexpectedBehaviorText ===
null
) {
delete unexpectedBehavior.otherUnexpectedBehaviorText;

updateParams.testResults[j] =
testPlanRunData[i].testResults[j];

needsUpdate = true;
}
}
}
}

if (needsUpdate) {
// eslint-disable-next-line no-console
console.info(
`=== Fixing unexpectedBehavior results for TestPlanRun:${testPlanRunId} ===`
);
await updateTestPlanRun(testPlanRunId, updateParams);
}
}

for (let i = 0; i < testPlanReportsData.length; i++) {
const testPlanReportId = testPlanReportsData[i].id;
const status = testPlanReportsData[i].status;
if (status === 'DRAFT') {
let updateParams = {};
const { testPlanReport } = await populateData(
{
testPlanReportId
},
{}
);

const conflicts = await conflictsResolver(
testPlanReport,
null,
{ atLoader, browserLoader }
Copy link
Contributor

@howard-e howard-e Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Just a thought, and not suggesting as something to act on as it's outside the scope of this PR.

Right now, the GQL context could be { user, atLoader, browserLoader } with the change in #623. Now that I've seen its usage a bit more, it seems atLoader and browserLoader being present is a must in most queries.

So I'd think that when context is null in the resolvers it's called, it may be preferred if it simply defaults to { atLoader, browserLoader }. I'm unsure if sure if there's any implications to that though, and also acknowledge that it would a considerable effort in determining that. But creating the objects here to solely retrieve the conflict results here felt out of place.

);

updateParams = {
metrics: {
conflictsCount: conflicts.length
}
};

await updateTestPlanReport(testPlanReport.id, updateParams);
}
}
});
}
};
14 changes: 14 additions & 0 deletions server/resolvers/TestResultOperations/saveTestResultCommon.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,20 @@ const saveTestResultCommon = async ({
removeArrayItems: true
});

// Some clients might send an otherUnexpectedBehaviorText for unexpectedBehaviors
// that are not "OTHER". As long as the otherUnexpectedBehaviorText is null or undefined,
// the best course of action is probably to allow it, but not save it to the database.
newTestResult.scenarioResults.forEach(scenarioResult => {
scenarioResult.unexpectedBehaviors.forEach(unexpectedBehavior => {
if (
unexpectedBehavior.id !== 'OTHER' &&
unexpectedBehavior.otherUnexpectedBehaviorText == null
) {
delete unexpectedBehavior.otherUnexpectedBehaviorText;
}
});
});

const isCorrupted = !deepPickEqual(
[
createTestResultSkeleton({ test, testPlanRun, testPlanReport }),
Expand Down