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<pranaykarvi@gmail.com>

* 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 <3614296+davetsay@users.noreply.github.com>
This commit is contained in:
Andrew Henry 2025-05-14 09:09:03 -07:00 committed by GitHub
parent fffee68e9c
commit 2667ff6a4e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
9 changed files with 237 additions and 90 deletions

View File

@ -27,7 +27,8 @@ demonstrate some playwright for test developers. This pattern should not be re-u
import {
createDomainObjectWithDefaults,
createExampleTelemetryObject
createExampleTelemetryObject,
setRealTimeMode
} from '../../../../appActions.js';
import { expect, test } from '../../../../pluginFixtures.js';
@ -116,7 +117,7 @@ test.describe('Basic Condition Set Use', () => {
await page.getByLabel('Conditions View').click();
await expect(page.getByText('Current Output')).toBeVisible();
});
test('ConditionSet has correct outputs when telemetry is and is not available', async ({
test('ConditionSet produces an output when telemetry is available, and does not when it is not', async ({
page
}) => {
const exampleTelemetry = await createExampleTelemetryObject(page);
@ -281,6 +282,101 @@ test.describe('Basic Condition Set Use', () => {
await page.goto(exampleTelemetry.url);
});
test('Short circuit evaluation does not cause incorrect evaluation https://github.com/nasa/openmct/issues/7992', async ({
page
}) => {
await setRealTimeMode(page);
await page.getByLabel('Create', { exact: true }).click();
await page.getByLabel('State Generator').click();
await page.getByLabel('Title', { exact: true }).fill('P1');
await page.getByLabel('State Duration (seconds)').fill('1');
await page.getByLabel('Save').click();
await page.getByLabel('Create', { exact: true }).click();
await page.getByLabel('State Generator').click();
await page.getByLabel('Title', { exact: true }).fill('P2');
await page.getByLabel('State Duration (seconds)', { exact: true }).fill('1');
await page.getByRole('treeitem', { name: 'Test Condition Set' }).click();
await page.getByLabel('Save').click();
await page.getByLabel('Expand My Items folder').click();
await page.getByRole('treeitem', { name: 'Test Condition Set' }).click();
await page.getByLabel('Edit Object').click();
await page.getByLabel('Add Condition').click();
await page.getByLabel('Condition Name Input').first().fill('P1 IS ON AND P2 IS ON');
await page.getByLabel('Criterion Telemetry Selection').selectOption({ label: 'P1' });
await page.getByLabel('Criterion Metadata Selection').selectOption('value');
await page.getByLabel('Criterion Comparison Selection').selectOption('equalTo');
await page.getByLabel('Criterion Input').fill('1');
await page.getByLabel('Add Criteria - Enabled').click();
await page.getByLabel('Criterion Telemetry Selection').nth(1).selectOption({ label: 'P2' });
await page.getByLabel('Criterion Metadata Selection').nth(1).selectOption('value');
await page.getByLabel('Criterion Comparison Selection').nth(1).selectOption('equalTo');
await page.getByLabel('Criterion Input').nth(1).fill('1');
await page.getByLabel('Add Condition').click();
await page.getByLabel('Condition Name Input').first().fill('P1 IS OFF OR P2 IS OFF');
await page.getByLabel('Condition Trigger').first().selectOption('any');
await page.getByLabel('Criterion Telemetry Selection').first().selectOption({ label: 'P1' });
await page.getByLabel('Criterion Metadata Selection').first().selectOption('value');
await page.getByLabel('Criterion Comparison Selection').first().selectOption('equalTo');
await page.getByLabel('Criterion Input').first().fill('0');
await page.getByLabel('Add Criteria - Enabled').first().click();
await page.getByLabel('Criterion Telemetry Selection').nth(1).selectOption({ label: 'P2' });
await page.getByLabel('Criterion Metadata Selection').nth(1).selectOption('value');
await page.getByLabel('Criterion Comparison Selection').nth(1).selectOption('equalTo');
await page.getByLabel('Criterion Input').nth(1).fill('0');
await page.getByLabel('Condition Name Input').first().dblclick();
await page.getByLabel('Save').click();
await page.getByRole('listitem', { name: 'Save and Finish Editing' }).click();
await page.getByLabel('Edit Object').click();
/**
* Create default conditions for test. Start with invalid values to put condition set into
* "default" state
*/
await page.getByLabel('Test Data Telemetry Selection').selectOption({ label: 'P1' });
await page.getByLabel('Test Data Metadata Selection').selectOption({ label: 'Value' });
await page.getByLabel('Test Data Input').fill('3');
await page.getByLabel('Add Test Datum').click();
await page.getByLabel('Test Data Telemetry Selection').nth(1).selectOption({ label: 'P2' });
await page.getByLabel('Test Data Metadata Selection').nth(1).selectOption({ label: 'Value' });
await page.getByLabel('Test Data Input').nth(1).fill('3');
await page.getByLabel('Apply Test Data').nth(1).click();
let activeCondition = page.getByLabel('Active Condition Set Condition');
let activeConditionName = activeCondition.getByLabel('Condition Name Label');
await expect(activeConditionName).toHaveText('Default');
/**
* Set P1 to 0
*/
await page.getByLabel('Test Data Input').nth(0).fill('0');
activeCondition = page.getByLabel('Active Condition Set Condition');
activeConditionName = activeCondition.getByLabel('Condition Name Label');
await expect(activeConditionName).toHaveText('P1 IS OFF OR P2 IS OFF');
/**
* Set P2 to 1
*/
await page.getByLabel('Test Data Input').nth(1).fill('1');
activeCondition = page.getByLabel('Active Condition Set Condition');
activeConditionName = activeCondition.getByLabel('Condition Name Label');
await expect(activeConditionName).toHaveText('P1 IS OFF OR P2 IS OFF');
/**
* Set P1 to 1
*/
await page.getByLabel('Test Data Input').nth(0).fill('1');
activeCondition = page.getByLabel('Active Condition Set Condition');
activeConditionName = activeCondition.getByLabel('Condition Name Label');
await expect(activeConditionName).toHaveText('P1 IS ON AND P2 IS ON');
});
test.fixme('Ensure condition sets work with telemetry like operator status', ({ page }) => {
test.info().annotations.push({
type: 'issue',

View File

@ -44,48 +44,57 @@ import { getLatestTimestamp } from './utils/time.js';
* }
*/
export default class Condition extends EventEmitter {
#definition;
/**
* Manages criteria and emits the result of - true or false - based on criteria evaluated.
* @constructor
* @param conditionConfiguration: {id: uuid,trigger: enum, criteria: Array of {id: uuid, operation: enum, input: Array, metaDataKey: string, key: {domainObject.identifier} }
* @param definition: {id: uuid,trigger: enum, criteria: Array of {id: uuid, operation: enum, input: Array, metaDataKey: string, key: {domainObject.identifier} }
* @param openmct
* @param conditionManager
*/
constructor(conditionConfiguration, openmct, conditionManager) {
constructor(definition, openmct, conditionManager) {
super();
this.openmct = openmct;
this.conditionManager = conditionManager;
this.id = conditionConfiguration.id;
this.criteria = [];
this.result = undefined;
this.timeSystems = this.openmct.time.getAllTimeSystems();
if (conditionConfiguration.configuration.criteria) {
this.createCriteria(conditionConfiguration.configuration.criteria);
this.#definition = definition;
if (definition.configuration.criteria) {
this.createCriteria(definition.configuration.criteria);
}
this.trigger = conditionConfiguration.configuration.trigger;
this.trigger = definition.configuration.trigger;
this.summary = '';
this.handleCriterionUpdated = this.handleCriterionUpdated.bind(this);
this.handleOldTelemetryCriterion = this.handleOldTelemetryCriterion.bind(this);
this.handleTelemetryStaleness = this.handleTelemetryStaleness.bind(this);
}
get id() {
return this.#definition.id;
}
get configuration() {
return this.#definition.configuration;
}
updateResult(datum) {
if (!datum || !datum.id) {
updateResult(latestDataTable, telemetryIdThatChanged) {
if (!latestDataTable) {
console.log('no data received');
return;
}
// if all the criteria in this condition have no telemetry, we want to force the condition result to evaluate
if (this.hasNoTelemetry() || this.isTelemetryUsed(datum.id)) {
if (this.hasNoTelemetry() || this.isTelemetryUsed(telemetryIdThatChanged)) {
const currentTimeSystemKey = this.openmct.time.getTimeSystem().key;
this.criteria.forEach((criterion) => {
if (this.isAnyOrAllTelemetry(criterion)) {
criterion.updateResult(datum, this.conditionManager.telemetryObjects);
criterion.updateResult(latestDataTable, this.conditionManager.telemetryObjects);
} else {
if (criterion.usesTelemetry(datum.id)) {
criterion.updateResult(datum);
const relevantDatum = latestDataTable.get(criterion.telemetryObjectIdAsString);
if (criterion.shouldUpdateResult(relevantDatum, currentTimeSystemKey)) {
criterion.updateResult(relevantDatum, currentTimeSystemKey);
}
}
});
@ -102,9 +111,11 @@ export default class Condition extends EventEmitter {
}
hasNoTelemetry() {
return this.criteria.every((criterion) => {
return !this.isAnyOrAllTelemetry(criterion) && criterion.telemetry === '';
const usesSomeTelemetry = this.criteria.some((criterion) => {
return this.isAnyOrAllTelemetry(criterion) || criterion.telemetry !== '';
});
return !usesSomeTelemetry;
}
isTelemetryUsed(id) {
@ -182,7 +193,7 @@ export default class Condition extends EventEmitter {
findCriterion(id) {
let criterion;
for (let i = 0, ii = this.criteria.length; i < ii; i++) {
for (let i = 0; i < this.criteria.length; i++) {
if (this.criteria[i].id === id) {
criterion = {
item: this.criteria[i],
@ -247,7 +258,7 @@ export default class Condition extends EventEmitter {
this.timeSystems,
this.openmct.time.getTimeSystem()
);
this.conditionManager.updateCurrentCondition(latestTimestamp);
this.conditionManager.updateCurrentCondition(latestTimestamp, this);
}
handleTelemetryStaleness() {

View File

@ -27,6 +27,12 @@ import Condition from './Condition.js';
import { getLatestTimestamp } from './utils/time.js';
export default class ConditionManager extends EventEmitter {
#latestDataTable = new Map();
/**
* @param {import('openmct.js').DomainObject} conditionSetDomainObject
* @param {import('openmct.js').OpenMCT} openmct
*/
constructor(conditionSetDomainObject, openmct) {
super();
this.openmct = openmct;
@ -304,22 +310,6 @@ export default class ConditionManager extends EventEmitter {
this.persistConditions();
}
getCurrentCondition() {
const conditionCollection = this.conditionSetDomainObject.configuration.conditionCollection;
let currentCondition = conditionCollection[conditionCollection.length - 1];
for (let i = 0; i < conditionCollection.length - 1; i++) {
const condition = this.findConditionById(conditionCollection[i].id);
if (condition.result) {
//first condition to be true wins
currentCondition = conditionCollection[i];
break;
}
}
return currentCondition;
}
getCurrentConditionLAD(conditionResults) {
const conditionCollection = this.conditionSetDomainObject.configuration.conditionCollection;
let currentCondition = conditionCollection[conditionCollection.length - 1];
@ -410,26 +400,34 @@ export default class ConditionManager extends EventEmitter {
const normalizedDatum = this.createNormalizedDatum(datum, endpoint);
const timeSystemKey = this.openmct.time.getTimeSystem().key;
let timestamp = {};
const currentTimestamp = normalizedDatum[timeSystemKey];
const timestamp = {};
timestamp[timeSystemKey] = currentTimestamp;
this.#latestDataTable.set(normalizedDatum.id, normalizedDatum);
if (this.shouldEvaluateNewTelemetry(currentTimestamp)) {
this.updateConditionResults(normalizedDatum);
this.updateCurrentCondition(timestamp);
const matchingCondition = this.updateConditionResults(normalizedDatum.id);
this.updateCurrentCondition(timestamp, matchingCondition);
}
}
updateConditionResults(normalizedDatum) {
updateConditionResults(keyStringForUpdatedTelemetryObject) {
//We want to stop when the first condition evaluates to true.
this.conditions.some((condition) => {
condition.updateResult(normalizedDatum);
const matchingCondition = this.conditions.find((condition) => {
condition.updateResult(this.#latestDataTable, keyStringForUpdatedTelemetryObject);
return condition.result === true;
});
return matchingCondition;
}
updateCurrentCondition(timestamp) {
const currentCondition = this.getCurrentCondition();
updateCurrentCondition(timestamp, matchingCondition) {
const conditionCollection = this.conditionSetDomainObject.configuration.conditionCollection;
const defaultCondition = conditionCollection[conditionCollection.length - 1];
const currentCondition = matchingCondition || defaultCondition;
this.emit(
'conditionSetResultUpdated',
@ -444,11 +442,13 @@ export default class ConditionManager extends EventEmitter {
);
}
getTestData(metadatum) {
getTestData(metadatum, identifier) {
let data = undefined;
if (this.testData.applied) {
const found = this.testData.conditionTestInputs.find(
(testInput) => testInput.metadata === metadatum.source
(testInput) =>
testInput.metadata === metadatum.source &&
this.openmct.objects.areIdsEqual(testInput.telemetry, identifier)
);
if (found) {
data = found.value;
@ -463,9 +463,9 @@ export default class ConditionManager extends EventEmitter {
const metadata = this.openmct.telemetry.getMetadata(endpoint).valueMetadatas;
const normalizedDatum = Object.values(metadata).reduce((datum, metadatum) => {
const testValue = this.getTestData(metadatum);
const testValue = this.getTestData(metadatum, endpoint.identifier);
const formatter = this.openmct.telemetry.getValueFormatter(metadatum);
datum[metadatum.key] =
datum[metadatum.source || metadatum.key] =
testValue !== undefined
? formatter.parse(testValue)
: formatter.parse(telemetryDatum[metadatum.source]);
@ -480,7 +480,7 @@ export default class ConditionManager extends EventEmitter {
updateTestData(testData) {
if (!_.isEqual(testData, this.testData)) {
this.testData = testData;
this.testData = JSON.parse(JSON.stringify(testData));
this.openmct.objects.mutate(
this.conditionSetDomainObject,
'configuration.conditionTestData',

View File

@ -53,6 +53,7 @@ describe('The condition', function () {
valueMetadatas: [
{
key: 'some-key',
source: 'some-key',
name: 'Some attribute',
hints: {
range: 2
@ -60,6 +61,7 @@ describe('The condition', function () {
},
{
key: 'utc',
source: 'utc',
name: 'Time',
format: 'utc',
hints: {
@ -88,17 +90,32 @@ describe('The condition', function () {
openmct.telemetry = jasmine.createSpyObj('telemetry', [
'isTelemetryObject',
'subscribe',
'getMetadata'
'getMetadata',
'getValueFormatter'
]);
openmct.telemetry.isTelemetryObject.and.returnValue(true);
openmct.telemetry.subscribe.and.returnValue(function () {});
openmct.telemetry.getMetadata.and.returnValue(testTelemetryObject.telemetry);
openmct.telemetry.getValueFormatter.and.callFake((metadatum) => {
return {
parse(input) {
return input;
}
};
});
mockTimeSystems = {
key: 'utc'
};
openmct.time = jasmine.createSpyObj('time', ['getAllTimeSystems', 'on', 'off']);
openmct.time = jasmine.createSpyObj('time', [
'getTimeSystem',
'getAllTimeSystems',
'on',
'off'
]);
openmct.time.getTimeSystem.and.returnValue({ key: 'utc' });
openmct.time.getAllTimeSystems.and.returnValue([mockTimeSystems]);
//openmct.time.getTimeSystem.and.returnValue();
openmct.time.on.and.returnValue(() => {});
openmct.time.off.and.returnValue(() => {});
@ -113,7 +130,7 @@ describe('The condition', function () {
id: '1234-5678-9999-0000',
operation: 'equalTo',
input: ['0'],
metadata: 'value',
metadata: 'testSource',
telemetry: testTelemetryObject.identifier
}
]
@ -156,37 +173,24 @@ describe('The condition', function () {
expect(conditionObj.criteria.length).toEqual(0);
});
it('gets the result of a condition when new telemetry data is received', function () {
conditionObj.updateResult({
value: '0',
utc: 'Hi',
id: testTelemetryObject.identifier.key
});
expect(conditionObj.result).toBeTrue();
});
it('gets the result of a condition when new telemetry data is received', function () {
conditionObj.updateResult({
value: '1',
utc: 'Hi',
id: testTelemetryObject.identifier.key
});
expect(conditionObj.result).toBeFalse();
});
it('keeps the old result new telemetry data is not used by it', function () {
conditionObj.updateResult({
const latestDataTable = new Map();
latestDataTable.set(testTelemetryObject.identifier.key, {
value: '0',
utc: 'Hi',
id: testTelemetryObject.identifier.key
});
conditionObj.updateResult(latestDataTable, testTelemetryObject.identifier.key);
expect(conditionObj.result).toBeTrue();
conditionObj.updateResult({
latestDataTable.set('1234', {
value: '1',
utc: 'Hi',
id: '1234'
});
conditionObj.updateResult(latestDataTable, '1234');
expect(conditionObj.result).toBeTrue();
});
});

View File

@ -24,7 +24,7 @@
<div
class="c-condition-h"
:class="{ 'is-drag-target': draggingOver }"
aria-label="Condition Set Condition"
:aria-label="conditionSetLabel"
@dragover.prevent
@drop.prevent="dropCondition($event, conditionIndex)"
@dragenter="dragEnter($event, conditionIndex)"
@ -53,7 +53,9 @@
@click="expanded = !expanded"
></span>
<span class="c-condition__name">{{ condition.configuration.name }}</span>
<span class="c-condition__name" aria-label="Condition Name Label">{{
condition.configuration.name
}}</span>
<span class="c-condition__summary">
<template v-if="!condition.isDefault && !canEvaluateCriteria"> Define criteria </template>
<span v-else>
@ -259,6 +261,17 @@ export default {
};
},
computed: {
conditionSetLabel() {
let label;
if (this.condition.id === this.currentConditionId) {
label = 'Active Condition Set Condition';
} else {
label = 'Condition Set Condition';
}
return label;
},
triggers() {
const keys = Object.keys(TRIGGER);
const triggerOptions = [];

View File

@ -114,7 +114,7 @@
class="c-button c-button--major icon-plus labeled"
@click="addTestInput"
>
<span class="c-cs-button__label">Add Test Datum</span>
<span class="c-cs-button__label" aria-label="Add Test Datum">Add Test Datum</span>
</button>
</div>
</section>

View File

@ -181,13 +181,20 @@ export default class AllTelemetryCriterion extends TelemetryCriterion {
if (validatedData && !this.isStalenessCheck()) {
if (this.isOldCheck()) {
if (this.ageCheck?.[validatedData.id]) {
this.ageCheck[validatedData.id].update(validatedData);
}
Object.keys(this.telemetryDataCache).forEach((objectIdKeystring) => {
if (this.ageCheck?.[objectIdKeystring]) {
this.ageCheck[objectIdKeystring].update(validatedData[objectIdKeystring]);
}
this.telemetryDataCache[validatedData.id] = false;
this.telemetryDataCache[objectIdKeystring] = false;
});
} else {
this.telemetryDataCache[validatedData.id] = this.computeResult(validatedData);
Object.keys(this.telemetryDataCache).forEach((objectIdKeystring) => {
const telemetryObject = telemetryObjects[objectIdKeystring];
this.telemetryDataCache[objectIdKeystring] = this.computeResult(
this.createNormalizedDatum(validatedData[objectIdKeystring], telemetryObject)
);
});
}
}

View File

@ -29,21 +29,29 @@ import { getOperatorText, OPERATIONS } from '../utils/operations.js';
import { checkIfOld } from '../utils/time.js';
export default class TelemetryCriterion extends EventEmitter {
#lastUpdated;
#lastTimeSystem;
#comparator;
/**
* Subscribes/Unsubscribes to telemetry and emits the result
* of operations performed on the telemetry data returned and a given input value.
* @constructor
* @param telemetryDomainObjectDefinition {id: uuid, operation: enum, input: Array, metadata: string, key: {domainObject.identifier} }
* @param openmct
* @param {import('../../../MCT.js').OpenMCT} openmct
*/
constructor(telemetryDomainObjectDefinition, openmct) {
super();
/**
* @type {import('../../../MCT.js').MCT}
*/
this.openmct = openmct;
this.telemetryDomainObjectDefinition = telemetryDomainObjectDefinition;
this.id = telemetryDomainObjectDefinition.id;
this.telemetry = telemetryDomainObjectDefinition.telemetry;
this.operation = telemetryDomainObjectDefinition.operation;
this.#comparator = this.#findOperation(this.operation);
this.input = telemetryDomainObjectDefinition.input;
this.metadata = telemetryDomainObjectDefinition.metadata;
this.result = undefined;
@ -83,7 +91,6 @@ export default class TelemetryCriterion extends EventEmitter {
if (this.ageCheck) {
this.ageCheck.clear();
}
this.ageCheck = checkIfOld(this.handleOldTelemetry.bind(this), this.input[0] * 1000);
}
@ -153,7 +160,6 @@ export default class TelemetryCriterion extends EventEmitter {
createNormalizedDatum(telemetryDatum, endpoint) {
const id = this.openmct.objects.makeKeyString(endpoint.identifier);
const metadata = this.openmct.telemetry.getMetadata(endpoint).valueMetadatas;
const normalizedDatum = Object.values(metadata).reduce((datum, metadatum) => {
const formatter = this.openmct.telemetry.getValueFormatter(metadatum);
datum[metadatum.key] = formatter.parse(telemetryDatum[metadatum.source]);
@ -179,9 +185,18 @@ export default class TelemetryCriterion extends EventEmitter {
return datum;
}
shouldUpdateResult(datum, timesystem) {
const dataIsDefined = datum !== undefined;
const hasTimeSystemChanged =
this.#lastTimeSystem === undefined || this.#lastTimeSystem !== timesystem;
const isCacheStale = this.#lastUpdated === undefined || datum[timesystem] > this.#lastUpdated;
updateResult(data) {
const validatedData = this.isValid() ? data : {};
return dataIsDefined && (hasTimeSystemChanged || isCacheStale);
}
updateResult(data, currentTimeSystemKey) {
const validatedData = this.isValid()
? this.createNormalizedDatum(data, this.telemetryObject)
: {};
if (!this.isStalenessCheck()) {
if (this.isOldCheck()) {
@ -193,6 +208,8 @@ export default class TelemetryCriterion extends EventEmitter {
} else {
this.result = this.computeResult(validatedData);
}
this.#lastUpdated = data[currentTimeSystemKey];
this.#lastTimeSystem = currentTimeSystemKey;
}
}
@ -236,8 +253,8 @@ export default class TelemetryCriterion extends EventEmitter {
});
}
findOperation(operation) {
for (let i = 0, ii = OPERATIONS.length; i < ii; i++) {
#findOperation(operation) {
for (let i = 0; i < OPERATIONS.length; i++) {
if (operation === OPERATIONS[i].name) {
return OPERATIONS[i].operation;
}
@ -249,15 +266,14 @@ export default class TelemetryCriterion extends EventEmitter {
computeResult(data) {
let result = false;
if (data) {
let comparator = this.findOperation(this.operation);
let params = [];
params.push(data[this.metadata]);
if (this.isValidInput()) {
this.input.forEach((input) => params.push(input));
}
if (typeof comparator === 'function') {
result = Boolean(comparator(params));
if (typeof this.#comparator === 'function') {
result = Boolean(this.#comparator(params));
}
}

View File

@ -106,7 +106,7 @@ describe('The telemetry criterion', function () {
id: 'test-criterion-id',
telemetry: openmct.objects.makeKeyString(testTelemetryObject.identifier),
operation: 'textContains',
metadata: 'value',
metadata: 'testSource',
input: ['Hell'],
telemetryObjects: { [testTelemetryObject.identifier.key]: testTelemetryObject }
};