diff --git a/test/ci/ui_tests.sh b/test/ci/ui_tests.sh index 883245f281..00567d408a 100755 --- a/test/ci/ui_tests.sh +++ b/test/ci/ui_tests.sh @@ -38,12 +38,17 @@ set +e ui/run-integrationtests --out ${OUT_PATH} --no-build RES=$? +set +x + # Copy the output of screenshots diff testing. if [ -d ${OUT_PATH}/ui-test-artifacts ]; then cp -a ${OUT_PATH}/ui-test-artifacts /ci/artifacts/ui-test-artifacts echo "UI integration test report with screnshots:" echo "https://storage.googleapis.com/perfetto-ci-artifacts/$PERFETTO_TEST_JOB/ui-test-artifacts/index.html" echo "" + echo "To download locally the changed screenshots run:" + echo "tools/download_changed_screenshots.py $PERFETTO_TEST_JOB" + echo "" echo "Perfetto UI build for this CL" echo "https://storage.googleapis.com/perfetto-ci-artifacts/$PERFETTO_TEST_JOB/ui/index.html" exit $RES diff --git a/test/data/ui-screenshots/independent_features.test.ts/trace-error-notification/error-icon.png.sha256 b/test/data/ui-screenshots/independent_features.test.ts/trace-error-notification/error-icon.png.sha256 new file mode 100644 index 0000000000..7a64aaf69f --- /dev/null +++ b/test/data/ui-screenshots/independent_features.test.ts/trace-error-notification/error-icon.png.sha256 @@ -0,0 +1 @@ +d11a2d89b1d96ede01644accc98cee5ac6fadb15a13d40baa78977fd5c212670 \ No newline at end of file diff --git a/ui/src/common/fake_trace_impl.ts b/ui/src/common/fake_trace_impl.ts index f97dea54df..65d563622a 100644 --- a/ui/src/common/fake_trace_impl.ts +++ b/ui/src/common/fake_trace_impl.ts @@ -40,6 +40,7 @@ export function createFakeTraceImpl(args: FakeTraceImplArgs = {}) { traceTzOffset: Time.ZERO, cpus: [], gpuCount: 0, + importErrors: 0, }; return AppImpl.instance.newTraceInstance( new FakeEngine(args.allowQueries ?? false), diff --git a/ui/src/controller/trace_controller.ts b/ui/src/controller/trace_controller.ts index ebaf7333ed..dd64207413 100644 --- a/ui/src/controller/trace_controller.ts +++ b/ui/src/controller/trace_controller.ts @@ -50,9 +50,8 @@ import { WasmEngineProxy, } from '../trace_processor/wasm_engine_proxy'; import {showModal} from '../widgets/modal'; -import {Child, Children, Controller} from './controller'; +import {Controller} from './controller'; import {LoadingManager} from './loading_manager'; -import {TraceErrorController} from './trace_error_controller'; import { TraceBufferStream, TraceFileStream, @@ -227,15 +226,7 @@ export class TraceController extends Controller { break; case 'ready': - // At this point we are ready to serve queries and handle tracks. - const engine = assertExists(this.engine); - const childControllers: Children = []; - - childControllers.push( - Child('traceError', TraceErrorController, {engine}), - ); - - return childControllers; + return []; default: throw new Error(`unknown state ${this.state}`); @@ -1111,6 +1102,7 @@ async function getTraceTimeDetails( traceTzOffset, cpus: await getCpus(engine), gpuCount: await getNumberOfGpus(engine), + importErrors: await getTraceErrors(engine), source: engineCfg.source, }; } @@ -1147,6 +1139,12 @@ async function getNumberOfGpus(engine: Engine): Promise { return result.firstRow({gpuCount: NUM}).gpuCount; } +async function getTraceErrors(engine: Engine): Promise { + const sql = `SELECT sum(value) as errs FROM stats WHERE severity != 'info'`; + const result = await engine.query(sql); + return result.firstRow({errs: NUM}).errs; +} + async function getTracingMetadataTimeBounds(engine: Engine): Promise { const queryRes = await engine.query(`select name, diff --git a/ui/src/controller/trace_error_controller.ts b/ui/src/controller/trace_error_controller.ts deleted file mode 100644 index 0fde7b9ee8..0000000000 --- a/ui/src/controller/trace_error_controller.ts +++ /dev/null @@ -1,45 +0,0 @@ -// Copyright (C) 2020 The Android Open Source Project -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -import {publishTraceErrors} from '../frontend/publish'; -import {Engine} from '../trace_processor/engine'; -import {NUM} from '../trace_processor/query_result'; -import {Controller} from './controller'; - -export interface TraceErrorControllerArgs { - engine: Engine; -} - -export class TraceErrorController extends Controller<'main'> { - private hasRun = false; - constructor(private args: TraceErrorControllerArgs) { - super('main'); - } - - run() { - if (this.hasRun) { - return; - } - this.hasRun = true; - const engine = this.args.engine; - engine - .query( - `SELECT sum(value) as sumValue FROM stats WHERE severity != 'info'`, - ) - .then((result) => { - const errors = result.firstRow({sumValue: NUM}).sumValue; - publishTraceErrors(errors); - }); - } -} diff --git a/ui/src/frontend/globals.ts b/ui/src/frontend/globals.ts index e0b7e7f878..de45cf6cc4 100644 --- a/ui/src/frontend/globals.ts +++ b/ui/src/frontend/globals.ts @@ -92,7 +92,6 @@ class Globals { private _numQueriesQueued = 0; private _bufferUsage?: number = undefined; private _recordingLog?: string = undefined; - private _traceErrors?: number = undefined; private _metricError?: string = undefined; private _jobStatus?: Map = undefined; private _embeddedMode?: boolean = undefined; @@ -102,7 +101,6 @@ class Globals { httpRpcState: HttpRpcState = {connected: false}; showPanningHint = false; permalinkHash?: string; - showTraceErrorPopup = true; extraSqlPackages: SqlPackage[] = []; get workspace(): Workspace { @@ -248,14 +246,6 @@ class Globals { return assertExists(this._threadMap); } - get traceErrors() { - return this._traceErrors; - } - - setTraceErrors(arg: number) { - this._traceErrors = arg; - } - get metricError() { return this._metricError; } diff --git a/ui/src/frontend/publish.ts b/ui/src/frontend/publish.ts index ba02af607a..4d34714c00 100644 --- a/ui/src/frontend/publish.ts +++ b/ui/src/frontend/publish.ts @@ -77,11 +77,6 @@ export function publishRecordingLog(args: {logs: string}) { globals.publishRedraw(); } -export function publishTraceErrors(numErrors: number) { - globals.setTraceErrors(numErrors); - globals.publishRedraw(); -} - export function publishMetricError(error: string) { globals.setMetricError(error); globals.publishRedraw(); diff --git a/ui/src/frontend/topbar.ts b/ui/src/frontend/topbar.ts index 80c3cc67a7..0711b2cb16 100644 --- a/ui/src/frontend/topbar.ts +++ b/ui/src/frontend/topbar.ts @@ -21,6 +21,7 @@ import {Popup, PopupPosition} from '../widgets/popup'; import {assertFalse} from '../base/logging'; import {OmniboxMode} from '../core/omnibox_manager'; import {AppImpl} from '../core/app_trace_impl'; +import {Trace, TraceAttrs} from '../public/trace'; export const DISMISSED_PANNING_HINT_KEY = 'dismissedPanningHint'; @@ -75,12 +76,15 @@ class HelpPanningNotification implements m.ClassComponent { } } -class TraceErrorIcon implements m.ClassComponent { - view() { +class TraceErrorIcon implements m.ClassComponent { + private tracePopupErrorDismissed = false; + + view({attrs}: m.CVnode) { + const trace = attrs.trace; if (globals.embeddedMode) return; const mode = AppImpl.instance.omnibox.mode; - const errors = globals.traceErrors; + const errors = trace.traceInfo.importErrors; if ( (!Boolean(errors) && !globals.metricError) || mode === OmniboxMode.Command @@ -96,11 +100,11 @@ class TraceErrorIcon implements m.ClassComponent { Popup, { trigger: m('.popup-trigger'), - isOpen: globals.showTraceErrorPopup, + isOpen: !this.tracePopupErrorDismissed, position: PopupPosition.Left, onChange: (shouldOpen: boolean) => { assertFalse(shouldOpen); - globals.showTraceErrorPopup = false; + this.tracePopupErrorDismissed = true; }, }, m('.error-popup', 'Data-loss/import error. Click for more info.'), @@ -122,6 +126,7 @@ class TraceErrorIcon implements m.ClassComponent { export interface TopbarAttrs { omnibox: m.Children; + trace?: Trace; } export class Topbar implements m.ClassComponent { @@ -133,7 +138,7 @@ export class Topbar implements m.ClassComponent { omnibox, m(Progress), m(HelpPanningNotification), - m(TraceErrorIcon), + attrs.trace && m(TraceErrorIcon, {trace: attrs.trace}), ); } } diff --git a/ui/src/frontend/ui_main.ts b/ui/src/frontend/ui_main.ts index e11d9f4589..def4d1e396 100644 --- a/ui/src/frontend/ui_main.ts +++ b/ui/src/frontend/ui_main.ts @@ -677,6 +677,7 @@ export class UiMainPerTrace implements m.ClassComponent { m(Sidebar), m(Topbar, { omnibox: this.renderOmnibox(), + trace: this.trace, }), m(Alerts), children, diff --git a/ui/src/public/trace_info.ts b/ui/src/public/trace_info.ts index 11d0cf64c8..d48a5f726c 100644 --- a/ui/src/public/trace_info.ts +++ b/ui/src/public/trace_info.ts @@ -41,6 +41,9 @@ export interface TraceInfo { // The number of gpus in the trace readonly gpuCount: number; + + // The number of import/analysis errors present in the `stats` table. + readonly importErrors: number; } export interface TraceFileSource { diff --git a/ui/src/test/independent_features.test.ts b/ui/src/test/independent_features.test.ts index 329493ce33..c761ded883 100644 --- a/ui/src/test/independent_features.test.ts +++ b/ui/src/test/independent_features.test.ts @@ -32,3 +32,12 @@ test('debuggable chip', async ({browser}) => { await pth.toggleTrackGroup(trackGroup); await pth.waitForIdleAndScreenshot('track_with_debuggable_chip_expanded.png'); }); + +test('trace error notification', async ({browser}) => { + const page = await browser.newPage(); + const pth = new PerfettoTestHelper(page); + await pth.openTraceFile('clusterfuzz_14753'); + await pth.waitForIdleAndScreenshot('error-icon.png', { + clip: {x: 1800, y: 0, width: 150, height: 150}, + }); +}); diff --git a/ui/src/test/perfetto_ui_test_helper.ts b/ui/src/test/perfetto_ui_test_helper.ts index 22f153995f..a709f1fd67 100644 --- a/ui/src/test/perfetto_ui_test_helper.ts +++ b/ui/src/test/perfetto_ui_test_helper.ts @@ -63,7 +63,6 @@ export class PerfettoTestHelper { assertExists(file).setInputFiles(tracePath); await this.waitForPerfettoIdle(); await this.page.mouse.move(0, 0); - await this.page.mouse.click(0, 0); } waitForPerfettoIdle(idleHysteresisMs?: number): Promise {