Skip to content

Commit 2667ff6

Browse files
akhenrydavetsay
akhenry
and
davetsay
authored
cherry-pick #8041 - Condition Sets can incorrectly evaluate telemetry objects that update infre (#8064)
Condition Sets can incorrectly evaluate telemetry objects that update infrequently (#8041) * compares latest available data properly for condition calculations * Added timestamp checking for individual criteria * Co-authored-by: Pranaykarvi<[email protected]> * Fixed bug with test data * Replaced legacy tests with new e2e test of correct telemetry evaluation * Fixed long-standing bug with evaluating enums --------- Co-authored-by: David Tsay <[email protected]>
1 parent fffee68 commit 2667ff6

File tree

9 files changed

+237
-90
lines changed

9 files changed

+237
-90
lines changed

‎e2e/tests/functional/plugins/conditionSet/conditionSetOperations.e2e.spec.js‎

Lines changed: 98 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,8 @@ demonstrate some playwright for test developers. This pattern should not be re-u
2727

2828
import {
2929
createDomainObjectWithDefaults,
30-
createExampleTelemetryObject
30+
createExampleTelemetryObject,
31+
setRealTimeMode
3132
} from '../../../../appActions.js';
3233
import { expect, test } from '../../../../pluginFixtures.js';
3334

@@ -116,7 +117,7 @@ test.describe('Basic Condition Set Use', () => {
116117
await page.getByLabel('Conditions View').click();
117118
await expect(page.getByText('Current Output')).toBeVisible();
118119
});
119-
test('ConditionSet has correct outputs when telemetry is and is not available', async ({
120+
test('ConditionSet produces an output when telemetry is available, and does not when it is not', async ({
120121
page
121122
}) => {
122123
const exampleTelemetry = await createExampleTelemetryObject(page);
@@ -281,6 +282,101 @@ test.describe('Basic Condition Set Use', () => {
281282
await page.goto(exampleTelemetry.url);
282283
});
283284

285+
test('Short circuit evaluation does not cause incorrect evaluation https://github.com/nasa/openmct/issues/7992', async ({
286+
page
287+
}) => {
288+
await setRealTimeMode(page);
289+
await page.getByLabel('Create', { exact: true }).click();
290+
await page.getByLabel('State Generator').click();
291+
await page.getByLabel('Title', { exact: true }).fill('P1');
292+
await page.getByLabel('State Duration (seconds)').fill('1');
293+
await page.getByLabel('Save').click();
294+
await page.getByLabel('Create', { exact: true }).click();
295+
await page.getByLabel('State Generator').click();
296+
await page.getByLabel('Title', { exact: true }).fill('P2');
297+
await page.getByLabel('State Duration (seconds)', { exact: true }).fill('1');
298+
await page.getByRole('treeitem', { name: 'Test Condition Set' }).click();
299+
await page.getByLabel('Save').click();
300+
await page.getByLabel('Expand My Items folder').click();
301+
await page.getByRole('treeitem', { name: 'Test Condition Set' }).click();
302+
await page.getByLabel('Edit Object').click();
303+
await page.getByLabel('Add Condition').click();
304+
await page.getByLabel('Condition Name Input').first().fill('P1 IS ON AND P2 IS ON');
305+
await page.getByLabel('Criterion Telemetry Selection').selectOption({ label: 'P1' });
306+
await page.getByLabel('Criterion Metadata Selection').selectOption('value');
307+
await page.getByLabel('Criterion Comparison Selection').selectOption('equalTo');
308+
await page.getByLabel('Criterion Input').fill('1');
309+
await page.getByLabel('Add Criteria - Enabled').click();
310+
await page.getByLabel('Criterion Telemetry Selection').nth(1).selectOption({ label: 'P2' });
311+
await page.getByLabel('Criterion Metadata Selection').nth(1).selectOption('value');
312+
await page.getByLabel('Criterion Comparison Selection').nth(1).selectOption('equalTo');
313+
await page.getByLabel('Criterion Input').nth(1).fill('1');
314+
await page.getByLabel('Add Condition').click();
315+
await page.getByLabel('Condition Name Input').first().fill('P1 IS OFF OR P2 IS OFF');
316+
await page.getByLabel('Condition Trigger').first().selectOption('any');
317+
await page.getByLabel('Criterion Telemetry Selection').first().selectOption({ label: 'P1' });
318+
await page.getByLabel('Criterion Metadata Selection').first().selectOption('value');
319+
await page.getByLabel('Criterion Comparison Selection').first().selectOption('equalTo');
320+
await page.getByLabel('Criterion Input').first().fill('0');
321+
await page.getByLabel('Add Criteria - Enabled').first().click();
322+
await page.getByLabel('Criterion Telemetry Selection').nth(1).selectOption({ label: 'P2' });
323+
await page.getByLabel('Criterion Metadata Selection').nth(1).selectOption('value');
324+
await page.getByLabel('Criterion Comparison Selection').nth(1).selectOption('equalTo');
325+
await page.getByLabel('Criterion Input').nth(1).fill('0');
326+
await page.getByLabel('Condition Name Input').first().dblclick();
327+
await page.getByLabel('Save').click();
328+
await page.getByRole('listitem', { name: 'Save and Finish Editing' }).click();
329+
await page.getByLabel('Edit Object').click();
330+
331+
/**
332+
* Create default conditions for test. Start with invalid values to put condition set into
333+
* "default" state
334+
*/
335+
await page.getByLabel('Test Data Telemetry Selection').selectOption({ label: 'P1' });
336+
await page.getByLabel('Test Data Metadata Selection').selectOption({ label: 'Value' });
337+
await page.getByLabel('Test Data Input').fill('3');
338+
await page.getByLabel('Add Test Datum').click();
339+
await page.getByLabel('Test Data Telemetry Selection').nth(1).selectOption({ label: 'P2' });
340+
await page.getByLabel('Test Data Metadata Selection').nth(1).selectOption({ label: 'Value' });
341+
await page.getByLabel('Test Data Input').nth(1).fill('3');
342+
await page.getByLabel('Apply Test Data').nth(1).click();
343+
344+
let activeCondition = page.getByLabel('Active Condition Set Condition');
345+
let activeConditionName = activeCondition.getByLabel('Condition Name Label');
346+
347+
await expect(activeConditionName).toHaveText('Default');
348+
349+
/**
350+
* Set P1 to 0
351+
*/
352+
await page.getByLabel('Test Data Input').nth(0).fill('0');
353+
354+
activeCondition = page.getByLabel('Active Condition Set Condition');
355+
activeConditionName = activeCondition.getByLabel('Condition Name Label');
356+
357+
await expect(activeConditionName).toHaveText('P1 IS OFF OR P2 IS OFF');
358+
359+
/**
360+
* Set P2 to 1
361+
*/
362+
await page.getByLabel('Test Data Input').nth(1).fill('1');
363+
364+
activeCondition = page.getByLabel('Active Condition Set Condition');
365+
activeConditionName = activeCondition.getByLabel('Condition Name Label');
366+
367+
await expect(activeConditionName).toHaveText('P1 IS OFF OR P2 IS OFF');
368+
369+
/**
370+
* Set P1 to 1
371+
*/
372+
await page.getByLabel('Test Data Input').nth(0).fill('1');
373+
374+
activeCondition = page.getByLabel('Active Condition Set Condition');
375+
activeConditionName = activeCondition.getByLabel('Condition Name Label');
376+
377+
await expect(activeConditionName).toHaveText('P1 IS ON AND P2 IS ON');
378+
});
379+
284380
test.fixme('Ensure condition sets work with telemetry like operator status', ({ page }) => {
285381
test.info().annotations.push({
286382
type: 'issue',

‎src/plugins/condition/Condition.js‎

Lines changed: 28 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -44,48 +44,57 @@ import { getLatestTimestamp } from './utils/time.js';
4444
* }
4545
*/
4646
export default class Condition extends EventEmitter {
47+
#definition;
4748
/**
4849
* Manages criteria and emits the result of - true or false - based on criteria evaluated.
4950
* @constructor
50-
* @param conditionConfiguration: {id: uuid,trigger: enum, criteria: Array of {id: uuid, operation: enum, input: Array, metaDataKey: string, key: {domainObject.identifier} }
51+
* @param definition: {id: uuid,trigger: enum, criteria: Array of {id: uuid, operation: enum, input: Array, metaDataKey: string, key: {domainObject.identifier} }
5152
* @param openmct
5253
* @param conditionManager
5354
*/
54-
constructor(conditionConfiguration, openmct, conditionManager) {
55+
constructor(definition, openmct, conditionManager) {
5556
super();
5657

5758
this.openmct = openmct;
5859
this.conditionManager = conditionManager;
59-
this.id = conditionConfiguration.id;
6060
this.criteria = [];
6161
this.result = undefined;
6262
this.timeSystems = this.openmct.time.getAllTimeSystems();
63-
if (conditionConfiguration.configuration.criteria) {
64-
this.createCriteria(conditionConfiguration.configuration.criteria);
63+
this.#definition = definition;
64+
65+
if (definition.configuration.criteria) {
66+
this.createCriteria(definition.configuration.criteria);
6567
}
6668

67-
this.trigger = conditionConfiguration.configuration.trigger;
69+
this.trigger = definition.configuration.trigger;
6870
this.summary = '';
6971
this.handleCriterionUpdated = this.handleCriterionUpdated.bind(this);
7072
this.handleOldTelemetryCriterion = this.handleOldTelemetryCriterion.bind(this);
7173
this.handleTelemetryStaleness = this.handleTelemetryStaleness.bind(this);
7274
}
75+
get id() {
76+
return this.#definition.id;
77+
}
78+
get configuration() {
79+
return this.#definition.configuration;
80+
}
7381

74-
updateResult(datum) {
75-
if (!datum || !datum.id) {
82+
updateResult(latestDataTable, telemetryIdThatChanged) {
83+
if (!latestDataTable) {
7684
console.log('no data received');
77-
7885
return;
7986
}
8087

8188
// if all the criteria in this condition have no telemetry, we want to force the condition result to evaluate
82-
if (this.hasNoTelemetry() || this.isTelemetryUsed(datum.id)) {
89+
if (this.hasNoTelemetry() || this.isTelemetryUsed(telemetryIdThatChanged)) {
90+
const currentTimeSystemKey = this.openmct.time.getTimeSystem().key;
8391
this.criteria.forEach((criterion) => {
8492
if (this.isAnyOrAllTelemetry(criterion)) {
85-
criterion.updateResult(datum, this.conditionManager.telemetryObjects);
93+
criterion.updateResult(latestDataTable, this.conditionManager.telemetryObjects);
8694
} else {
87-
if (criterion.usesTelemetry(datum.id)) {
88-
criterion.updateResult(datum);
95+
const relevantDatum = latestDataTable.get(criterion.telemetryObjectIdAsString);
96+
if (criterion.shouldUpdateResult(relevantDatum, currentTimeSystemKey)) {
97+
criterion.updateResult(relevantDatum, currentTimeSystemKey);
8998
}
9099
}
91100
});
@@ -102,9 +111,11 @@ export default class Condition extends EventEmitter {
102111
}
103112

104113
hasNoTelemetry() {
105-
return this.criteria.every((criterion) => {
106-
return !this.isAnyOrAllTelemetry(criterion) && criterion.telemetry === '';
114+
const usesSomeTelemetry = this.criteria.some((criterion) => {
115+
return this.isAnyOrAllTelemetry(criterion) || criterion.telemetry !== '';
107116
});
117+
118+
return !usesSomeTelemetry;
108119
}
109120

110121
isTelemetryUsed(id) {
@@ -182,7 +193,7 @@ export default class Condition extends EventEmitter {
182193
findCriterion(id) {
183194
let criterion;
184195

185-
for (let i = 0, ii = this.criteria.length; i < ii; i++) {
196+
for (let i = 0; i < this.criteria.length; i++) {
186197
if (this.criteria[i].id === id) {
187198
criterion = {
188199
item: this.criteria[i],
@@ -247,7 +258,7 @@ export default class Condition extends EventEmitter {
247258
this.timeSystems,
248259
this.openmct.time.getTimeSystem()
249260
);
250-
this.conditionManager.updateCurrentCondition(latestTimestamp);
261+
this.conditionManager.updateCurrentCondition(latestTimestamp, this);
251262
}
252263

253264
handleTelemetryStaleness() {

‎src/plugins/condition/ConditionManager.js‎

Lines changed: 29 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,12 @@ import Condition from './Condition.js';
2727
import { getLatestTimestamp } from './utils/time.js';
2828

2929
export default class ConditionManager extends EventEmitter {
30+
#latestDataTable = new Map();
31+
32+
/**
33+
* @param {import('openmct.js').DomainObject} conditionSetDomainObject
34+
* @param {import('openmct.js').OpenMCT} openmct
35+
*/
3036
constructor(conditionSetDomainObject, openmct) {
3137
super();
3238
this.openmct = openmct;
@@ -304,22 +310,6 @@ export default class ConditionManager extends EventEmitter {
304310
this.persistConditions();
305311
}
306312

307-
getCurrentCondition() {
308-
const conditionCollection = this.conditionSetDomainObject.configuration.conditionCollection;
309-
let currentCondition = conditionCollection[conditionCollection.length - 1];
310-
311-
for (let i = 0; i < conditionCollection.length - 1; i++) {
312-
const condition = this.findConditionById(conditionCollection[i].id);
313-
if (condition.result) {
314-
//first condition to be true wins
315-
currentCondition = conditionCollection[i];
316-
break;
317-
}
318-
}
319-
320-
return currentCondition;
321-
}
322-
323313
getCurrentConditionLAD(conditionResults) {
324314
const conditionCollection = this.conditionSetDomainObject.configuration.conditionCollection;
325315
let currentCondition = conditionCollection[conditionCollection.length - 1];
@@ -410,26 +400,34 @@ export default class ConditionManager extends EventEmitter {
410400

411401
const normalizedDatum = this.createNormalizedDatum(datum, endpoint);
412402
const timeSystemKey = this.openmct.time.getTimeSystem().key;
413-
let timestamp = {};
414403
const currentTimestamp = normalizedDatum[timeSystemKey];
404+
const timestamp = {};
405+
415406
timestamp[timeSystemKey] = currentTimestamp;
407+
this.#latestDataTable.set(normalizedDatum.id, normalizedDatum);
408+
416409
if (this.shouldEvaluateNewTelemetry(currentTimestamp)) {
417-
this.updateConditionResults(normalizedDatum);
418-
this.updateCurrentCondition(timestamp);
410+
const matchingCondition = this.updateConditionResults(normalizedDatum.id);
411+
this.updateCurrentCondition(timestamp, matchingCondition);
419412
}
420413
}
421414

422-
updateConditionResults(normalizedDatum) {
415+
updateConditionResults(keyStringForUpdatedTelemetryObject) {
423416
//We want to stop when the first condition evaluates to true.
424-
this.conditions.some((condition) => {
425-
condition.updateResult(normalizedDatum);
417+
const matchingCondition = this.conditions.find((condition) => {
418+
condition.updateResult(this.#latestDataTable, keyStringForUpdatedTelemetryObject);
426419

427420
return condition.result === true;
428421
});
422+
423+
return matchingCondition;
429424
}
430425

431-
updateCurrentCondition(timestamp) {
432-
const currentCondition = this.getCurrentCondition();
426+
updateCurrentCondition(timestamp, matchingCondition) {
427+
const conditionCollection = this.conditionSetDomainObject.configuration.conditionCollection;
428+
const defaultCondition = conditionCollection[conditionCollection.length - 1];
429+
430+
const currentCondition = matchingCondition || defaultCondition;
433431

434432
this.emit(
435433
'conditionSetResultUpdated',
@@ -444,11 +442,13 @@ export default class ConditionManager extends EventEmitter {
444442
);
445443
}
446444

447-
getTestData(metadatum) {
445+
getTestData(metadatum, identifier) {
448446
let data = undefined;
449447
if (this.testData.applied) {
450448
const found = this.testData.conditionTestInputs.find(
451-
(testInput) => testInput.metadata === metadatum.source
449+
(testInput) =>
450+
testInput.metadata === metadatum.source &&
451+
this.openmct.objects.areIdsEqual(testInput.telemetry, identifier)
452452
);
453453
if (found) {
454454
data = found.value;
@@ -463,9 +463,9 @@ export default class ConditionManager extends EventEmitter {
463463
const metadata = this.openmct.telemetry.getMetadata(endpoint).valueMetadatas;
464464

465465
const normalizedDatum = Object.values(metadata).reduce((datum, metadatum) => {
466-
const testValue = this.getTestData(metadatum);
466+
const testValue = this.getTestData(metadatum, endpoint.identifier);
467467
const formatter = this.openmct.telemetry.getValueFormatter(metadatum);
468-
datum[metadatum.key] =
468+
datum[metadatum.source || metadatum.key] =
469469
testValue !== undefined
470470
? formatter.parse(testValue)
471471
: formatter.parse(telemetryDatum[metadatum.source]);
@@ -480,7 +480,7 @@ export default class ConditionManager extends EventEmitter {
480480

481481
updateTestData(testData) {
482482
if (!_.isEqual(testData, this.testData)) {
483-
this.testData = testData;
483+
this.testData = JSON.parse(JSON.stringify(testData));
484484
this.openmct.objects.mutate(
485485
this.conditionSetDomainObject,
486486
'configuration.conditionTestData',

0 commit comments

Comments
 (0)