diff --git a/e2e/README.md b/e2e/README.md
index 9702bae04e..052a188325 100644
--- a/e2e/README.md
+++ b/e2e/README.md
@@ -89,7 +89,7 @@ Read more about [Playwright Snapshots](https://playwright.dev/docs/test-snapshot
#### Open MCT's implementation
- Our Snapshot tests receive a `@snapshot` tag.
-- Snapshots need to be executed within the official Playwright container to ensure we're using the exact rendering platform in CI and locally.
+- Snapshots need to be executed within the official Playwright container to ensure we're using the exact rendering platform in CI and locally. To do a valid comparison locally:
```sh
docker run --rm --network host -v $(pwd):/work/ -w /work/ -it mcr.microsoft.com/playwright:[GET THIS VERSION FROM OUR CIRCLECI CONFIG FILE]-focal /bin/bash
@@ -97,9 +97,24 @@ npm install
npx playwright test --config=e2e/playwright-ci.config.js --project=chrome --grep @snapshot
```
-### (WIP) Updating Snapshots
+### Updating Snapshots
-When the `@snapshot` tests fail, they will need to be evaluated to see if the failure is an acceptable change or
+When the `@snapshot` tests fail, they will need to be evaluated to determine if the failure is an acceptable and desireable or an unintended regression.
+
+To compare a snapshot, run a test and open the html report with the 'Expected' vs 'Actual' screenshot. If the actual screenshot is preferred, then the source-controlled 'Expected' snapshots will need to be updated with the following scripts.
+
+MacOS
+```
+npm run test:e2e:updatesnapshots
+```
+
+Linux/CI
+```sh
+// Replace {X.X.X} with the current Playwright version
+// from our package.json or circleCI configuration file
+docker run --rm --network host -v $(pwd):/work/ -w /work/ -it mcr.microsoft.com/playwright:v{X.X.X}-focal /bin/bash
+npm install
+npm run test:e2e:updatesnapshots
## Performance Testing
diff --git a/e2e/tests/functional/plugins/plot/autoscale.e2e.spec.js b/e2e/tests/functional/plugins/plot/autoscale.e2e.spec.js
index fb9a2e2a73..608fa03605 100644
--- a/e2e/tests/functional/plugins/plot/autoscale.e2e.spec.js
+++ b/e2e/tests/functional/plugins/plot/autoscale.e2e.spec.js
@@ -32,7 +32,7 @@ test.use({
}
});
-test.describe('ExportAsJSON', () => {
+test.describe('Autoscale', () => {
test('User can set autoscale with a valid range @snapshot', async ({ page, openmctConfig }) => {
const { myItemsFolderName } = openmctConfig;
@@ -47,16 +47,32 @@ test.describe('ExportAsJSON', () => {
await testYTicks(page, ['-1.00', '-0.50', '0.00', '0.50', '1.00']);
+ // enter edit mode
+ await page.click('button[title="Edit"]');
+
await turnOffAutoscale(page);
- // Make sure that after turning off autoscale, the user selected range values start at the same values the plot had prior.
- await testYTicks(page, ['-1.00', '-0.50', '0.00', '0.50', '1.00']);
+ await setUserDefinedMinAndMax(page, '-2', '2');
+
+ // save
+ await page.click('button[title="Save"]');
+ await Promise.all([
+ page.locator('li[title = "Save and Finish Editing"]').click(),
+ //Wait for Save Banner to appear
+ page.waitForSelector('.c-message-banner__message')
+ ]);
+ //Wait until Save Banner is gone
+ await page.locator('.c-message-banner__close-button').click();
+ await page.waitForSelector('.c-message-banner__message', { state: 'detached'});
+
+ // Make sure that after turning off autoscale, the user entered range values are reflexted in the ticks.
+ await testYTicks(page, ['-2.00', '-1.50', '-1.00', '-0.50', '0.00', '0.50', '1.00', '1.50', '2.00']);
const canvas = page.locator('canvas').nth(1);
await canvas.hover({trial: true});
- expect(await canvas.screenshot()).toMatchSnapshot('autoscale-canvas-prepan.png', { animations: 'disabled' });
+ expect.soft(await canvas.screenshot()).toMatchSnapshot('autoscale-canvas-prepan.png', { animations: 'disabled' });
//Alt Drag Start
await page.keyboard.down('Alt');
@@ -76,11 +92,12 @@ test.describe('ExportAsJSON', () => {
await page.keyboard.up('Alt');
// Ensure the drag worked.
- await testYTicks(page, ['0.00', '0.50', '1.00', '1.50', '2.00']);
+ await testYTicks(page, ['0.00', '0.50', '1.00', '1.50', '2.00', '2.50', '3.00', '3.50']);
+ //Wait for canvas to stablize.
await canvas.hover({trial: true});
- expect(await canvas.screenshot()).toMatchSnapshot('autoscale-canvas-panned.png', { animations: 'disabled' });
+ expect.soft(await canvas.screenshot()).toMatchSnapshot('autoscale-canvas-panned.png', { animations: 'disabled' });
});
});
@@ -152,22 +169,25 @@ async function createSinewaveOverlayPlot(page, myItemsFolderName) {
* @param {import('@playwright/test').Page} page
*/
async function turnOffAutoscale(page) {
- // enter edit mode
- await page.locator('text=Unnamed Overlay Plot Snapshot >> button').nth(3).click();
-
// uncheck autoscale
await page.getByRole('listitem').filter({ hasText: 'Auto scale' }).getByRole('checkbox').uncheck();
+}
- // save
- await page.locator('text=Snapshot Save and Finish Editing Save and Continue Editing >> button').nth(1).click();
- await Promise.all([
- page.locator('text=Save and Finish Editing').click(),
- //Wait for Save Banner to appear
- page.waitForSelector('.c-message-banner__message')
- ]);
- //Wait until Save Banner is gone
- await page.locator('.c-message-banner__close-button').click();
- await page.waitForSelector('.c-message-banner__message', { state: 'detached'});
+/**
+ * @param {import('@playwright/test').Page} page
+ * @param {string} min
+ * @param {string} max
+ */
+async function setUserDefinedMinAndMax(page, min, max) {
+ // set minimum value
+ const minRangeInput = page.getByRole('listitem').filter({ hasText: 'Minimum Value' }).locator('input[type="number"]');
+ await minRangeInput.click();
+ await minRangeInput.fill(min);
+
+ // set maximum value
+ const maxRangeInput = page.getByRole('listitem').filter({ hasText: 'Maximum Value' }).locator('input[type="number"]');
+ await maxRangeInput.click();
+ await maxRangeInput.fill(max);
}
/**
@@ -179,7 +199,7 @@ async function testYTicks(page, values) {
let promises = [yTicks.count().then(c => expect(c).toBe(values.length))];
for (let i = 0, l = values.length; i < l; i += 1) {
- promises.push(expect(yTicks.nth(i)).toHaveText(values[i])); // eslint-disable-line
+ promises.push(expect.soft(yTicks.nth(i)).toHaveText(values[i])); // eslint-disable-line
}
await Promise.all(promises);
diff --git a/e2e/tests/functional/plugins/plot/autoscale.e2e.spec.js-snapshots/autoscale-canvas-panned-chrome-darwin.png b/e2e/tests/functional/plugins/plot/autoscale.e2e.spec.js-snapshots/autoscale-canvas-panned-chrome-darwin.png
index d6d4dd21e5..e9e82fd14e 100644
Binary files a/e2e/tests/functional/plugins/plot/autoscale.e2e.spec.js-snapshots/autoscale-canvas-panned-chrome-darwin.png and b/e2e/tests/functional/plugins/plot/autoscale.e2e.spec.js-snapshots/autoscale-canvas-panned-chrome-darwin.png differ
diff --git a/e2e/tests/functional/plugins/plot/autoscale.e2e.spec.js-snapshots/autoscale-canvas-panned-chrome-linux.png b/e2e/tests/functional/plugins/plot/autoscale.e2e.spec.js-snapshots/autoscale-canvas-panned-chrome-linux.png
index 4a882660e0..c1119b8123 100644
Binary files a/e2e/tests/functional/plugins/plot/autoscale.e2e.spec.js-snapshots/autoscale-canvas-panned-chrome-linux.png and b/e2e/tests/functional/plugins/plot/autoscale.e2e.spec.js-snapshots/autoscale-canvas-panned-chrome-linux.png differ
diff --git a/e2e/tests/functional/plugins/plot/autoscale.e2e.spec.js-snapshots/autoscale-canvas-prepan-chrome-darwin.png b/e2e/tests/functional/plugins/plot/autoscale.e2e.spec.js-snapshots/autoscale-canvas-prepan-chrome-darwin.png
index ef5455e5c4..0c0e7c6e70 100644
Binary files a/e2e/tests/functional/plugins/plot/autoscale.e2e.spec.js-snapshots/autoscale-canvas-prepan-chrome-darwin.png and b/e2e/tests/functional/plugins/plot/autoscale.e2e.spec.js-snapshots/autoscale-canvas-prepan-chrome-darwin.png differ
diff --git a/e2e/tests/functional/plugins/plot/autoscale.e2e.spec.js-snapshots/autoscale-canvas-prepan-chrome-linux.png b/e2e/tests/functional/plugins/plot/autoscale.e2e.spec.js-snapshots/autoscale-canvas-prepan-chrome-linux.png
index ffdedffd2b..360f87b8f7 100644
Binary files a/e2e/tests/functional/plugins/plot/autoscale.e2e.spec.js-snapshots/autoscale-canvas-prepan-chrome-linux.png and b/e2e/tests/functional/plugins/plot/autoscale.e2e.spec.js-snapshots/autoscale-canvas-prepan-chrome-linux.png differ
diff --git a/src/plugins/plot/MctPlot.vue b/src/plugins/plot/MctPlot.vue
index ecdee217ff..ec24ea8fa6 100644
--- a/src/plugins/plot/MctPlot.vue
+++ b/src/plugins/plot/MctPlot.vue
@@ -855,7 +855,7 @@ export default {
gatherNearbyAnnotations() {
const nearbyAnnotations = [];
this.config.series.models.forEach(series => {
- if (series.closest.annotationsById) {
+ if (series?.closest?.annotationsById) {
Object.values(series.closest.annotationsById).forEach(closeAnnotation => {
const addedAnnotationAlready = nearbyAnnotations.some(annotation => {
return _.isEqual(annotation.targets, closeAnnotation.targets)
diff --git a/src/plugins/plot/chart/MctChart.vue b/src/plugins/plot/chart/MctChart.vue
index 58863d7c56..d4b2428bea 100644
--- a/src/plugins/plot/chart/MctChart.vue
+++ b/src/plugins/plot/chart/MctChart.vue
@@ -52,6 +52,41 @@ const MARKER_SIZE = 6.0;
const HIGHLIGHT_SIZE = MARKER_SIZE * 2.0;
const ANNOTATION_SIZE = MARKER_SIZE * 3.0;
const CLEARANCE = 15;
+// These attributes are changed in the plot model, but we don't need to react to the changes.
+const NO_HANDLING_NEEDED_ATTRIBUTES = {
+ label: 'label',
+ values: 'values',
+ format: 'format',
+ color: 'color',
+ name: 'name',
+ unit: 'unit'
+};
+// These attributes in turn set one of HANDLED_ATTRIBUTES, so we don't need specific listeners for them - this prevents excessive redraws.
+const IMPLICIT_HANDLED_ATTRIBUTES = {
+ range: 'range',
+ //series stats update y axis stats
+ stats: 'stats',
+ frozen: 'frozen',
+ autoscale: 'autoscale',
+ autoscalePadding: 'autoscalePadding',
+ logMode: 'logMode',
+ yKey: 'yKey'
+};
+// Attribute changes that we are specifically handling with listeners
+const HANDLED_ATTRIBUTES = {
+ //X and Y Axis attributes
+ key: 'key',
+ displayRange: 'displayRange',
+ //series attributes
+ xKey: 'xKey',
+ interpolate: 'interpolate',
+ markers: 'markers',
+ markerShape: 'markerShape',
+ markerSize: 'markerSize',
+ alarmMarkers: 'alarmMarkers',
+ limitLines: 'limitLines',
+ yAxisId: 'yAxisId'
+};
export default {
inject: ['openmct', 'domainObject', 'path'],
@@ -138,14 +173,16 @@ export default {
this.offset = {
[yAxisId]: {}
};
- this.listenTo(this.config.yAxis, 'change:key', this.resetYOffsetAndSeriesDataForYAxis.bind(this, yAxisId), this);
- this.listenTo(this.config.yAxis, 'change', this.updateLimitsAndDraw);
+ this.listenTo(this.config.yAxis, `change:${HANDLED_ATTRIBUTES.displayRange}`, this.scheduleDraw);
+ this.listenTo(this.config.yAxis, `change:${HANDLED_ATTRIBUTES.key}`, this.resetYOffsetAndSeriesDataForYAxis.bind(this, yAxisId), this);
+ this.listenTo(this.config.yAxis, 'change', this.redrawIfNotAlreadyHandled);
if (this.config.additionalYAxes.length) {
this.config.additionalYAxes.forEach(yAxis => {
const id = yAxis.get('id');
this.offset[id] = {};
- this.listenTo(yAxis, 'change', this.updateLimitsAndDraw);
- this.listenTo(yAxis, 'change:key', this.resetYOffsetAndSeriesDataForYAxis.bind(this, id), this);
+ this.listenTo(yAxis, `change:${HANDLED_ATTRIBUTES.displayRange}`, this.scheduleDraw);
+ this.listenTo(yAxis, `change:${HANDLED_ATTRIBUTES.key}`, this.resetYOffsetAndSeriesDataForYAxis.bind(this, id), this);
+ this.listenTo(yAxis, 'change', this.redrawIfNotAlreadyHandled);
});
}
@@ -162,7 +199,8 @@ export default {
this.listenTo(this.config.series, 'add', this.onSeriesAdd, this);
this.listenTo(this.config.series, 'remove', this.onSeriesRemove, this);
- this.listenTo(this.config.xAxis, 'change', this.updateLimitsAndDraw);
+ this.listenTo(this.config.xAxis, 'change:displayRange', this.scheduleDraw);
+ this.listenTo(this.config.xAxis, 'change', this.redrawIfNotAlreadyHandled);
this.config.series.forEach(this.onSeriesAdd, this);
this.$emit('chartLoaded');
},
@@ -191,14 +229,15 @@ export default {
this.changeLimitLines(mode, o, series);
},
onSeriesAdd(series) {
- this.listenTo(series, 'change:xKey', this.reDraw, this);
- this.listenTo(series, 'change:interpolate', this.changeInterpolate, this);
- this.listenTo(series, 'change:markers', this.changeMarkers, this);
- this.listenTo(series, 'change:alarmMarkers', this.changeAlarmMarkers, this);
- this.listenTo(series, 'change:limitLines', this.changeLimitLines, this);
- this.listenTo(series, 'change:yAxisId', this.resetAxisAndRedraw, this);
- // TODO: Which other changes is the listener below reacting to?
- this.listenTo(series, 'change', this.scheduleDraw);
+ this.listenTo(series, `change:${HANDLED_ATTRIBUTES.xKey}`, this.reDraw, this);
+ this.listenTo(series, `change:${HANDLED_ATTRIBUTES.interpolate}`, this.changeInterpolate, this);
+ this.listenTo(series, `change:${HANDLED_ATTRIBUTES.markers}`, this.changeMarkers, this);
+ this.listenTo(series, `change:${HANDLED_ATTRIBUTES.alarmMarkers}`, this.changeAlarmMarkers, this);
+ this.listenTo(series, `change:${HANDLED_ATTRIBUTES.limitLines}`, this.changeLimitLines, this);
+ this.listenTo(series, `change:${HANDLED_ATTRIBUTES.yAxisId}`, this.resetAxisAndRedraw, this);
+ this.listenTo(series, `change:${HANDLED_ATTRIBUTES.markerShape}`, this.scheduleDraw, this);
+ this.listenTo(series, `change:${HANDLED_ATTRIBUTES.markerSize}`, this.scheduleDraw, this);
+ this.listenTo(series, 'change', this.redrawIfNotAlreadyHandled);
this.listenTo(series, 'add', this.onAddPoint);
this.makeChartElement(series);
this.makeLimitLines(series);
@@ -531,8 +570,25 @@ export default {
return true;
},
+ redrawIfNotAlreadyHandled(attribute, value, oldValue) {
+ if (Object.keys(HANDLED_ATTRIBUTES).includes(attribute) && oldValue) {
+ return;
+ }
+
+ if (Object.keys(IMPLICIT_HANDLED_ATTRIBUTES).includes(attribute) && oldValue) {
+ return;
+ }
+
+ if (Object.keys(NO_HANDLING_NEEDED_ATTRIBUTES).includes(attribute) && oldValue) {
+ return;
+ }
+
+ console.warn('Unhandled change:', attribute);
+ this.updateLimitsAndDraw();
+ },
updateLimitsAndDraw() {
this.drawLimitLines();
+ this.scheduleDraw();
},
scheduleDraw() {
if (!this.drawScheduled) {
diff --git a/src/plugins/plot/configuration/PlotSeries.js b/src/plugins/plot/configuration/PlotSeries.js
index ac9a2ee724..7715004cc2 100644
--- a/src/plugins/plot/configuration/PlotSeries.js
+++ b/src/plugins/plot/configuration/PlotSeries.js
@@ -252,6 +252,7 @@ export default class PlotSeries extends Model {
}
const valueMetadata = this.metadata.value(newKey);
+ //TODO: Should we do this even if there is a persisted config?
if (!this.persistedConfig || !this.persistedConfig.interpolate) {
if (valueMetadata.format === 'enum') {
this.set('interpolate', 'stepAfter');
diff --git a/src/plugins/plot/configuration/YAxisModel.js b/src/plugins/plot/configuration/YAxisModel.js
index 1107b8e803..a111c9bc12 100644
--- a/src/plugins/plot/configuration/YAxisModel.js
+++ b/src/plugins/plot/configuration/YAxisModel.js
@@ -57,7 +57,14 @@ export default class YAxisModel extends Model {
this.listenTo(this, 'change:logMode', this.onLogModeChange, this);
this.listenTo(this, 'change:frozen', this.toggleFreeze, this);
this.listenTo(this, 'change:range', this.updateDisplayRange, this);
- this.updateDisplayRange(this.get('range'));
+ const range = this.get('range');
+ this.updateDisplayRange(range);
+ //This is an edge case and should not happen
+ const invalidRange = !range || (range?.min === undefined || range?.max === undefined);
+ const invalidAutoScaleOff = (options.model.autoscale === false) && invalidRange;
+ if (invalidAutoScaleOff) {
+ this.set('autoscale', true);
+ }
}
/**
* @param {import('./SeriesCollection').default} seriesCollection
@@ -250,23 +257,6 @@ export default class YAxisModel extends Model {
}
this.set('displayRange', _range);
- } else {
- // Otherwise use the last known displayRange as the initial
- // values for the user-defined range, so that we don't end up
- // with any error from an undefined user range.
-
- const _range = this.get('displayRange');
-
- if (!_range) {
- return;
- }
-
- if (this.get('logMode')) {
- _range.min = antisymlog(_range.min, 10);
- _range.max = antisymlog(_range.max, 10);
- }
-
- this.set('range', _range);
}
}
@@ -377,11 +367,8 @@ export default class YAxisModel extends Model {
autoscale: true,
logMode: options.model?.logMode ?? false,
autoscalePadding: 0.1,
- id: options.id
-
- // 'range' is not specified here, it is undefined at first. When the
- // user turns off autoscale, the current 'displayRange' is used for
- // the initial value of 'range'.
+ id: options.id,
+ range: options.model?.range
};
}
}
diff --git a/src/plugins/plot/inspector/PlotOptionsBrowse.vue b/src/plugins/plot/inspector/PlotOptionsBrowse.vue
index c99004d74a..54e6e46536 100644
--- a/src/plugins/plot/inspector/PlotOptionsBrowse.vue
+++ b/src/plugins/plot/inspector/PlotOptionsBrowse.vue
@@ -73,7 +73,7 @@
{{ yAxis.rangeMin }}
{
range = yAxis.get('range');
@@ -230,8 +230,8 @@ export default {
autoscale: yAxis.get('autoscale'),
logMode: yAxis.get('logMode'),
autoscalePadding: yAxis.get('autoscalePadding'),
- rangeMin: range ? range.min : '',
- rangeMax: range ? range.max : ''
+ rangeMin: range?.min ?? '',
+ rangeMax: range?.max ?? ''
});
});
}
diff --git a/src/plugins/plot/inspector/forms/YAxisForm.vue b/src/plugins/plot/inspector/forms/YAxisForm.vue
index e4a88ebe50..afdf481491 100644
--- a/src/plugins/plot/inspector/forms/YAxisForm.vue
+++ b/src/plugins/plot/inspector/forms/YAxisForm.vue
@@ -81,7 +81,7 @@
>Minimum Value
Maximum Value