fix summary graph selection persistence after xAxis switch
test-case: https://docs.google.com/document/d/1YYce6OV1qWWNOIgk1uKEX_jy7BwJQAh3T1EWWvY6KL8/edit?resourcekey=0-FbBWzqG6h_eWIg5rVQMFGg&tab=t.0#heading=h.164plq7xjf7i
Bug: 448352249
Change-Id: I95aae7c2c3b10b655e85f38f3e323b9cabae82c0
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/1075258
Commit-Queue: Sergei Rudenkov <sergeirudenkov@google.com>
Reviewed-by: Marcin Mordecki <mordeckimarcin@google.com>
diff --git a/infra-sk/modules/BUILD.bazel b/infra-sk/modules/BUILD.bazel
index 43433fb..e9cb61d 100644
--- a/infra-sk/modules/BUILD.bazel
+++ b/infra-sk/modules/BUILD.bazel
@@ -53,6 +53,8 @@
":object_ts_lib",
"//:node_modules/@types/chai",
"//:node_modules/chai",
+ "//:node_modules/lit",
+ "//infra-sk/modules/ElementSk:index_ts_lib",
],
)
diff --git a/infra-sk/modules/test_util.ts b/infra-sk/modules/test_util.ts
index c0ceb4f..1dbf8a8 100644
--- a/infra-sk/modules/test_util.ts
+++ b/infra-sk/modules/test_util.ts
@@ -9,6 +9,8 @@
import { expect } from 'chai';
import { deepCopy } from './object';
+import { LitElement } from 'lit';
+import { ElementSk } from './ElementSk';
/**
* Takes a DOM element name (e.g. 'my-component-sk') and returns a factory
@@ -314,3 +316,22 @@
export function setQueryString(q: string) {
window.history.pushState(null, '', window.location.origin + window.location.pathname + q);
}
+
+/**
+ * Waits for the next render cycle of a LitElement or ElementSk component, including browser paint.
+ * Useful for ensuring DOM updates have completed in tests.
+ *
+ * @param el The element to wait for.
+ */
+export async function waitForRender(el: ElementSk | LitElement) {
+ const domUpdate = () => new Promise((resolve) => setTimeout(resolve, 0));
+ if (!el) return;
+ await domUpdate();
+ if (el instanceof LitElement && el.updateComplete) {
+ await el.updateComplete;
+ }
+ // LitElement updates are done, now wait for browser to paint
+ await new Promise((resolve) => requestAnimationFrame(resolve));
+ // Final yield
+ await domUpdate();
+}
diff --git a/perf/modules/explore-simple-sk/BUILD.bazel b/perf/modules/explore-simple-sk/BUILD.bazel
index f9f87d5..a2500eb 100644
--- a/perf/modules/explore-simple-sk/BUILD.bazel
+++ b/perf/modules/explore-simple-sk/BUILD.bazel
@@ -80,6 +80,7 @@
src = "explore-simple-sk_test.ts",
deps = [
":explore-simple-sk",
+ "//:node_modules/@material/web",
"//:node_modules/@types/chai",
"//:node_modules/@types/sinon",
"//:node_modules/chai",
@@ -89,5 +90,6 @@
"//infra-sk/modules:test_util_ts_lib",
"//perf/modules/dataframe:test_utils_ts_lib",
"//perf/modules/json:index_ts_lib",
+ "//perf/modules/plot-summary-sk",
],
)
diff --git a/perf/modules/explore-simple-sk/explore-simple-sk.ts b/perf/modules/explore-simple-sk/explore-simple-sk.ts
index 78a5b04..27a7e6c 100644
--- a/perf/modules/explore-simple-sk/explore-simple-sk.ts
+++ b/perf/modules/explore-simple-sk/explore-simple-sk.ts
@@ -140,6 +140,9 @@
import { FavoritesDialogSk } from '../favorites-dialog-sk/favorites-dialog-sk';
import { CommitLinks } from '../point-links-sk/point-links-sk';
+const DOMAIN_DATE = 'date';
+const DOMAIN_COMMIT = 'commit';
+
/** The type of trace we are adding to a plot. */
type addPlotType = 'query' | 'formula' | 'pivot';
@@ -320,7 +323,7 @@
selected: PointSelected = defaultPointSelected(); // The point on a trace that was clicked on.
- domain: string = 'commit'; // The domain of the x-axis, either commit or date. Default commit.
+ domain: string = DOMAIN_COMMIT; // The domain of the x-axis, either commit or date. Default commit
// Deprecated.
labelMode: LabelMode = LabelMode.Date; // The label mode for the x-axis, date or commit.
@@ -1023,7 +1026,11 @@
return colHeader;
}
- private getCommitIndex(value: number, type: string = 'commit', max: boolean = false): number {
+ private getCommitIndex(
+ value: number,
+ type: string = DOMAIN_COMMIT,
+ max: boolean = false
+ ): number {
// Ensure the index value is an integer.
value = Math.round(value);
const header = this.dfRepo.value?.header;
@@ -1031,7 +1038,7 @@
return -1;
}
- const prop = type === 'commit' ? 'offset' : 'timestamp';
+ const prop = type === DOMAIN_COMMIT ? 'offset' : 'timestamp';
// First, try to find an exact match.
let index = header.findIndex((h) => h?.[prop] === value);
@@ -1356,25 +1363,18 @@
// If the switch is selected, set the x-axis to date mode.
switchXAxis(target: MdSwitch | null) {
if (!target) return;
- const newDomain = target.selected ? 'date' : 'commit';
- if (this._state.domain !== newDomain) {
- if (target.selected) {
- this._state.labelMode = LabelMode.Date;
- this._state.domain = 'date';
- } else {
- this._state.labelMode = LabelMode.CommitPosition;
- this._state.domain = 'commit';
- }
+ const newDomain = target.selected ? DOMAIN_DATE : DOMAIN_COMMIT;
- if (this.is_chart_split) {
- const detail = {
- index: this.state.graph_index,
- domain: this.state.domain,
- };
- this.dispatchEvent(new CustomEvent('x-axis-toggled', { detail, bubbles: true }));
- }
- this.updateXAxis(this._state.domain);
+ // Dispatch the event first to sync other charts.
+ if (this.is_chart_split) {
+ const detail = {
+ index: this.state.graph_index,
+ domain: newDomain,
+ };
+ this.dispatchEvent(new CustomEvent('x-axis-toggled', { detail, bubbles: true }));
}
+ // Then update the current chart.
+ this.updateXAxis(newDomain);
}
// Set the x-axis domain to either commit or date.
@@ -1382,10 +1382,17 @@
// It also is an entry point from multichart to update related charts which is
// why it has a separate method.
updateXAxis(domain: string) {
- if (this.state.domain !== domain) {
- this.xAxisSwitch = !this.xAxisSwitch;
- this._state.domain = domain;
+ if (this.state.domain === domain) {
+ return;
}
+
+ this._updateSelectionRangeForDomainChange(domain);
+
+ // Now, update the state and the child components' domains.
+ this.xAxisSwitch = !this.xAxisSwitch;
+ this._state.domain = domain;
+ this._state.labelMode = domain === DOMAIN_DATE ? LabelMode.Date : LabelMode.CommitPosition;
+
if (this.googleChartPlot.value) {
this.googleChartPlot.value.domain = domain as 'commit' | 'date';
}
@@ -1397,6 +1404,34 @@
this._stateHasChanged();
}
+ private _updateSelectionRangeForDomainChange(newDomain: string) {
+ const header = this.dfRepo.value?.header;
+ const plotSummary = this.plotSummary.value;
+ const oldDomain = this.state.domain;
+
+ if (header && plotSummary && plotSummary.selectedValueRange) {
+ const currentRange = plotSummary.selectedValueRange;
+
+ const beginIndex = this.getCommitIndex(currentRange.begin, oldDomain);
+ const endIndex = this.getCommitIndex(currentRange.end, oldDomain, true);
+
+ if (beginIndex !== -1 && endIndex !== -1) {
+ const newBegin =
+ newDomain === DOMAIN_DATE ? header[beginIndex]!.timestamp : header[beginIndex]!.offset;
+ const newEnd =
+ newDomain === DOMAIN_DATE ? header[endIndex]!.timestamp : header[endIndex]!.offset;
+ const newRange = { begin: newBegin, end: newEnd };
+
+ // Set the cached value on the child. It will apply this selection once
+ // its internal chart has re-rendered with the new domain.
+ (plotSummary as any).cachedSelectedValueRange = newRange;
+ if (this.googleChartPlot.value) {
+ this.googleChartPlot.value.selectedValueRange = newRange;
+ }
+ }
+ }
+ }
+
// updates the chart height using a string input.
// typically 500px for a single chart and 250px for multiple charts
updateChartHeight(height: string) {
@@ -1906,7 +1941,7 @@
let beginValue: number = beginCommit!.offset;
let endValue: number = endCommit!.offset;
- if (this.state.domain === 'date') {
+ if (this.state.domain === DOMAIN_DATE) {
beginValue = beginCommit!.timestamp;
endValue = endCommit!.timestamp;
}
diff --git a/perf/modules/explore-simple-sk/explore-simple-sk_test.ts b/perf/modules/explore-simple-sk/explore-simple-sk_test.ts
index 59e818d..b407608 100644
--- a/perf/modules/explore-simple-sk/explore-simple-sk_test.ts
+++ b/perf/modules/explore-simple-sk/explore-simple-sk_test.ts
@@ -9,6 +9,8 @@
TimestampSeconds,
Trace,
TraceSet,
+ DataFrame,
+ ReadOnlyParamSet,
} from '../json';
import { deepCopy } from '../../../infra-sk/modules/object';
import {
@@ -23,15 +25,53 @@
updateShortcut,
State,
} from './explore-simple-sk';
-import { setUpElementUnderTest } from '../../../infra-sk/modules/test_util';
+import { MdDialog } from '@material/web/dialog/dialog';
+import { MdSwitch } from '@material/web/switch/switch';
+import { PlotSummarySk } from '../plot-summary-sk/plot-summary-sk';
+import { setUpElementUnderTest, waitForRender } from '../../../infra-sk/modules/test_util';
import { generateFullDataFrame } from '../dataframe/test_utils';
import sinon from 'sinon';
+// Import for side effects. Make `plotSummary`(has no direct interaction with the module)
+// work when run in isolation.
+import './explore-simple-sk';
fetchMock.config.overwriteRoutes = true;
const now = 1726081856; // an arbitrary UNIX time;
const timeSpan = 89; // an arbitrary prime number for time span between commits .
+window.perf = {
+ instance_url: '',
+ radius: 2,
+ key_order: null,
+ num_shift: 50,
+ interesting: 2,
+ step_up_only: false,
+ commit_range_url: '',
+ demo: true,
+ display_group_by: false,
+ hide_list_of_commits_on_explore: false,
+ notifications: 'none',
+ fetch_chrome_perf_anomalies: false,
+ feedback_url: '',
+ chat_url: '',
+ help_url_override: '',
+ trace_format: '',
+ need_alert_action: false,
+ bug_host_url: '',
+ git_repo_url: '',
+ keys_for_commit_range: [],
+ keys_for_useful_links: [],
+ skip_commit_detail_display: false,
+ image_tag: 'fake-tag',
+ remove_default_stat_value: false,
+ enable_skia_bridge_aggregation: false,
+ show_json_file_display: false,
+ always_show_commit_info: false,
+ show_triage_link: true,
+ show_bisect_btn: true,
+};
+
describe('calculateRangeChange', () => {
const offsets: CommitRange = [100, 120] as CommitRange;
@@ -93,45 +133,6 @@
});
});
-// this function is needed to support other unit tests
-describe('applyFuncToTraces', () => {
- window.perf = {
- instance_url: '',
- radius: 2,
- key_order: null,
- num_shift: 50,
- interesting: 2,
- step_up_only: false,
- commit_range_url: '',
- demo: true,
- display_group_by: false,
- hide_list_of_commits_on_explore: false,
- notifications: 'none',
- fetch_chrome_perf_anomalies: false,
- feedback_url: '',
- chat_url: '',
- help_url_override: '',
- trace_format: '',
- need_alert_action: false,
- bug_host_url: '',
- git_repo_url: '',
- keys_for_commit_range: [],
- keys_for_useful_links: [],
- skip_commit_detail_display: false,
- image_tag: 'fake-tag',
- remove_default_stat_value: false,
- enable_skia_bridge_aggregation: false,
- show_json_file_display: false,
- always_show_commit_info: false,
- show_triage_link: true,
- show_bisect_btn: true,
- };
-
- // Create a common element-sk to be used by all the tests.
- const explore = document.createElement('explore-simple-sk') as ExploreSimpleSk;
- document.body.appendChild(explore);
-});
-
describe('PointSelected', () => {
it('defaults to not having a name', () => {
const p = defaultPointSelected();
@@ -698,3 +699,212 @@
);
});
});
+
+describe('x-axis domain switching', () => {
+ const INITIAL_TIMESTAMP_BEGIN = 1672531200;
+ const INITIAL_TIMESTAMP_END = 1672542000;
+ const COMMIT_101 = 101;
+ const COMMIT_102 = 102;
+ const TIMESTAMP_101 = 1672534800;
+ const TIMESTAMP_102 = 1672538400;
+ const ROUNDING_TOLERANCE_SECONDS = 120;
+ // A simple header for converting between commit offsets and timestamps.
+ const testHeader: ColumnHeader[] = [
+ {
+ offset: CommitNumber(100),
+ timestamp: TimestampSeconds(1672531200),
+ hash: 'h1',
+ author: '',
+ message: '',
+ url: '',
+ },
+ {
+ offset: CommitNumber(101),
+ timestamp: TimestampSeconds(1672534800),
+ hash: 'h2',
+ author: '',
+ message: '',
+ url: '',
+ },
+ {
+ offset: CommitNumber(102),
+ timestamp: TimestampSeconds(1672538400),
+ hash: 'h3',
+ author: '',
+ message: '',
+ url: '',
+ },
+ {
+ offset: CommitNumber(103),
+ timestamp: TimestampSeconds(1672542000),
+ hash: 'h4',
+ author: '',
+ message: '',
+ url: '',
+ },
+ ];
+
+ const testDataFrame: DataFrame = {
+ traceset: TraceSet({
+ ',config=test,': Trace([1, 2, 3, 4]),
+ }),
+ header: testHeader,
+ paramset: {} as ReadOnlyParamSet,
+ skip: 0,
+ traceMetadata: [],
+ };
+
+ const testFrameResponse: FrameResponse = {
+ dataframe: testDataFrame,
+ anomalymap: null,
+ display_mode: 'display_plot',
+ skps: [],
+ msg: '',
+ };
+
+ let explore: ExploreSimpleSk;
+
+ beforeEach(async () => {
+ explore = setUpElementUnderTest<ExploreSimpleSk>('explore-simple-sk')();
+ await window.customElements.whenDefined('explore-simple-sk');
+ await window.customElements.whenDefined('dataframe-repository-sk');
+ await window.customElements.whenDefined('plot-summary-sk');
+ });
+
+ // Helper function to set up the component for domain switching tests.
+ async function setupDomainSwitchTest(domain: 'date' | 'commit'): Promise<PlotSummarySk> {
+ explore.state = {
+ ...explore.state,
+ queries: ['config=test'],
+ domain: domain,
+ plotSummary: true,
+ begin: INITIAL_TIMESTAMP_BEGIN,
+ end: INITIAL_TIMESTAMP_END,
+ requestType: 0,
+ };
+ await waitForRender(explore);
+
+ // Provide data to the component.
+ await explore.UpdateWithFrameResponse(
+ testFrameResponse,
+ {
+ begin: explore.state.begin,
+ end: explore.state.end,
+ num_commits: 250,
+ request_type: 0,
+ formulas: [],
+ queries: ['config=test'],
+ keys: '',
+ tz: 'UTC',
+ pivot: null,
+ disable_filter_parent_traces: false,
+ },
+ false,
+ null,
+ false
+ );
+ await waitForRender(explore);
+
+ await new Promise((resolve) => setTimeout(resolve, 0));
+
+ const plotSummary = explore.querySelector('plot-summary-sk') as PlotSummarySk;
+ assert.exists(plotSummary, 'The plot-summary-sk element should be in the DOM.');
+ return plotSummary;
+ }
+
+ it('preserves selection when switching from date to commit', async () => {
+ const plotSummary = await setupDomainSwitchTest('date');
+
+ // Set an initial time-based selection on plot-summary-sk
+ const initialSelection = { begin: TIMESTAMP_101, end: TIMESTAMP_102 };
+ plotSummary.selectedValueRange = initialSelection;
+ await plotSummary.updateComplete;
+ await new Promise((resolve) => setTimeout(resolve, 0));
+
+ const settingsDialog = explore.querySelector('#settings-dialog') as MdDialog;
+ const switchEl = settingsDialog.querySelector('#commit-switch') as MdSwitch;
+ assert.exists(switchEl, '#commit-switch element not found.');
+
+ // Simulate switching to 'commit' domain (selected = false)
+ switchEl!.selected = false;
+ switchEl!.dispatchEvent(new Event('change'));
+
+ // Wait for ExploreSimpleSk to handle the change and update its children
+ await waitForRender(explore);
+ await plotSummary.updateComplete;
+ await new Promise((resolve) => setTimeout(resolve, 100));
+
+ // rare, but can be flaky here. Since it wait for async event.
+ // Increase of timeout above can help.
+ assert.exists(
+ plotSummary.selectedValueRange,
+ 'selectedValueRange should not be null after switch'
+ );
+
+ // Although the actual 'begin' and 'end' values are integers, they can be converted to
+ // floating-point numbers for UI rendering to prevent the graph from "jumping" when the x-axis
+ // domain is switched. The approximation in this test is used solely to prevent failures caused
+ // by floating-point arithmetic inaccuracies, e.g., 101 !== 101.000000001.
+ assert.approximately(
+ plotSummary.selectedValueRange.begin as number,
+ COMMIT_101,
+ 1e-3,
+ 'Selected range.begin did not convert correctly'
+ );
+
+ assert.approximately(
+ plotSummary.selectedValueRange.end as number,
+ COMMIT_102,
+ 1e-3,
+ 'Selected range.end did not convert correctly'
+ );
+
+ assert.equal(explore.state.domain, 'commit', 'Explore state domain should be commit');
+ assert.equal(plotSummary.domain, 'commit', 'PlotSummary domain property should be commit');
+ });
+
+ it('preserves selection when switching from commit to date', async () => {
+ const roundingToleranceSeconds = ROUNDING_TOLERANCE_SECONDS;
+ const plotSummary = await setupDomainSwitchTest('commit');
+
+ // Set an initial commit-based selection on plot-summary-sk
+ const initialSelection = { begin: COMMIT_101, end: COMMIT_102 };
+ plotSummary.selectedValueRange = initialSelection;
+ await plotSummary.updateComplete;
+ await new Promise((resolve) => setTimeout(resolve, 0));
+
+ const settingsDialog = explore.querySelector('#settings-dialog') as MdDialog;
+ const switchEl = settingsDialog.querySelector('#commit-switch') as MdSwitch;
+ assert.exists(switchEl, '#commit-switch element not found.');
+
+ // Simulate switching to 'date' domain (selected = true)
+ switchEl!.selected = true;
+ switchEl!.dispatchEvent(new Event('change'));
+
+ await waitForRender(explore);
+ await plotSummary.updateComplete;
+ await new Promise((resolve) => setTimeout(resolve, 100));
+
+ assert.exists(
+ plotSummary.selectedValueRange,
+ 'selectedValueRange should not be null after switch'
+ );
+
+ assert.approximately(
+ plotSummary.selectedValueRange.begin as number,
+ TIMESTAMP_101,
+ roundingToleranceSeconds,
+ 'Selected range.begin did not convert correctly'
+ );
+
+ assert.approximately(
+ plotSummary.selectedValueRange.end as number,
+ TIMESTAMP_102,
+ roundingToleranceSeconds,
+ 'Selected range.end did not convert correctly'
+ );
+
+ assert.equal(explore.state.domain, 'date', 'Explore state domain should be date');
+ assert.equal(plotSummary.domain, 'date', 'PlotSummary domain property should be date');
+ });
+});
