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.

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

Improved atVersion.supportedByAutomation field handling #1182

Merged
merged 4 commits into from
Jul 31, 2024
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
8 changes: 7 additions & 1 deletion client/components/AddTestToQueueWithConfirmation/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,13 @@ function AddTestToQueueWithConfirmation({
);

const existingTestPlanReports =
existingTestPlanReportsData?.existingTestPlanVersion?.testPlanReports;
existingTestPlanReportsData?.existingTestPlanVersion?.testPlanReports.filter(
tpr =>
tpr.at?.id === at?.id &&
tpr.browser?.id === browser?.id &&
tpr.minimumAtVersion?.id === minimumAtVersion?.id &&
tpr.exactAtVersion?.id === exactAtVersion?.id
Comment on lines +73 to +74
Copy link
Contributor

Choose a reason for hiding this comment

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

To the best of my understanding, only one of these may exist at a time so conditionally checking may be good, with exactAtVersion being the priority. Does that change your thoughts on this comparison?

I suppose it shouldn't matter though, since in the instance of the one (exact or minimum) that doesn't exist, undefined === undefined will be evaluated to true anyways anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think it still makes sense because we are ensuring the Reports are a match and so if one has an undefined minimum then the other would as well. One or the other is going to be a useless check since having only a minimum or a required is probably enforced elsewhere but I think it is a lower cognitive load to read than doing an OR between those two.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough on the cognitive load, this is fine with me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you!

);

let latestOldVersion;
let oldReportToCopyResultsFrom;
Expand Down
18 changes: 1 addition & 17 deletions server/models/AtVersion.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,3 @@
const {
AT_VERSIONS_SUPPORTED_BY_COLLECTION_JOBS
} = require('../util/constants');

const MODEL_NAME = 'AtVersion';

module.exports = function (sequelize, DataTypes) {
Expand Down Expand Up @@ -32,10 +28,7 @@ module.exports = function (sequelize, DataTypes) {
defaultValue: new Date()
},
supportedByAutomation: {
type: DataTypes.VIRTUAL,
get() {
return Model.isSupportedByAutomation(this.atId, this.name);
}
type: DataTypes.VIRTUAL
}
},
{
Expand All @@ -56,14 +49,5 @@ module.exports = function (sequelize, DataTypes) {
});
};

Model.isSupportedByAutomation = async function (atId, versionName) {
const At = sequelize.models.At;
const at = await At.findByPk(atId);
if (!at) return false;
const supportedVersions =
AT_VERSIONS_SUPPORTED_BY_COLLECTION_JOBS[at.name] || [];
return supportedVersions.includes(versionName);
};

return Model;
};
7 changes: 7 additions & 0 deletions server/resolvers/AtVersion/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
const supportedByAutomation = require('./supportedByAutomationResolver');

const AtVersion = {
supportedByAutomation
};

module.exports = AtVersion;
16 changes: 16 additions & 0 deletions server/resolvers/AtVersion/supportedByAutomationResolver.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
const { getAtVersionById } = require('../../models/services/AtService');
const {
isSupportedByAutomation
} = require('../../util/isSupportedByAutomation');

const supportedByAutomationResolver = async (parent, _, { transaction }) => {
const atVersion = await getAtVersionById({
id: parent.id,
transaction
});
return isSupportedByAutomation(atVersion.at.id, atVersion.name, {
transaction
});
};

module.exports = supportedByAutomationResolver;
4 changes: 3 additions & 1 deletion server/resolvers/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ const CollectionJob = require('./CollectionJob');
const TestPlanRun = require('./TestPlanRun');
const Test = require('./Test');
const ScenarioResult = require('./ScenarioResult');
const AtVersion = require('./AtVersion');

const resolvers = {
Query: {
Expand Down Expand Up @@ -97,7 +98,8 @@ const resolvers = {
TestPlanRunOperations,
TestResultOperations,
TestPlanVersionOperations,
CollectionJobOperations
CollectionJobOperations,
AtVersion
};

module.exports = resolvers;
23 changes: 9 additions & 14 deletions server/util/getAtVersionWithRequirements.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,11 @@ const getAtVersionWithRequirements = async (
);
}

const isMinimumVersionSupported =
await minimumAtVersion.supportedByAutomation;
if (isMinimumVersionSupported) {
if (minimumAtVersion.supportedByAutomation) {
return minimumAtVersion;
}

const matchingAts = await getAtVersions({
const matchingAtVersions = await getAtVersions({
where: {
atId,
releasedAt: { [Op.gte]: minimumAtVersion.releasedAt }
Expand All @@ -35,23 +33,20 @@ const getAtVersionWithRequirements = async (
transaction
});

const supportedAts = await Promise.all(
matchingAts.map(async version => {
const supportedByAutomation = await version.supportedByAutomation;
return supportedByAutomation ? version.toJSON() : null;
})
);

const latestSupportedAt = supportedAts.find(Boolean);
const latestSupportedAtVersion = matchingAtVersions.find(async atv => {
// supportedByAutomation is a computed graphql field,
// so we need to compute directly here
return atv.supportedByAutomation;
});

if (!latestSupportedAt) {
if (!latestSupportedAtVersion) {
throw new Error(
`No suitable AT version found for automation for AT ${atId} ` +
`with minimumAtVersion ${minimumAtVersion?.name}`
);
}

return latestSupportedAt;
return latestSupportedAtVersion;
} catch (error) {
console.error('Error while determining AT version:', error);
throw error;
Expand Down
30 changes: 30 additions & 0 deletions server/util/isSupportedByAutomation.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
const AtLoader = require('../models/loaders/AtLoader');
const { AT_VERSIONS_SUPPORTED_BY_COLLECTION_JOBS } = require('./constants');

let atIdToNameCache = {};

const isSupportedByAutomation = async function (
atId,
versionName,
{ transaction }
) {
if (Object.keys(atIdToNameCache).length === 0) {
const ats = await AtLoader().getAll({ transaction });
for (const at of ats) {
atIdToNameCache[at.id] = at.name;
}
}
const atName = atIdToNameCache[atId];
if (!atName) {
console.warn(
`Attempt to check if ${versionName} is supported by automation for unknown AT ${atId}`
);
return false;
}
const supportedVersions =
AT_VERSIONS_SUPPORTED_BY_COLLECTION_JOBS[atName] || [];
const isSupported = supportedVersions.includes(versionName);
return isSupported;
};

module.exports = { isSupportedByAutomation };
Loading