test: Fail CDP commands on unhandled exceptions #9639

martinpitt · 2018-07-11T08:10:28Z

Install a Chrome Devtools Protocol handler for unhandled exceptions, and
fail the next CDP command with it.

This got lost with the move from PhantomJS to Chrome.

adjust naughty override for testSerialConsole
investigate crash in testNfsMissingPackages
fix failures of tests which expect ph_* rejections
fix Cannot read property 'data_types' of null crash in test/verify/check-openshift TestOpenshift.testKubevirtMachinesList

martinpitt · 2018-07-11T08:12:30Z

Note: I did not include the playground test from #9539 yet, as this needs some more thought on how to turn this into a successful test. Let's first see which other tests cause an unhandled exception - it seems at least the serial console test should fail in a more useful way here.

stefwalter · 2018-07-11T10:51:02Z

test/common/cdp-driver.js

@@ -72,6 +73,8 @@ function setupLogging(client) {
        resolveLogPromise();
    });

+    client.Runtime.exceptionThrown(info => unhandledExceptions.push(info.exceptionDetails));


This should also be written to the console. Is it?

It could (and indeed I had a local test version that did that). It would replicate the same message as the testlib.Error, though, so it wouldn't tell anything new.

If there are multiple exceptions, I really want to see them all, not just the last one. Similar to what we do with unexpected journal messages.

martinpitt · 2018-07-12T06:50:17Z

test/common/cdp-driver.js

@@ -295,7 +298,7 @@ CDP.New(options)
                                    if (unhandledExceptions.length === 0) {
                                        success(reply);
                                    } else {
-                                        fail(unhandledExceptions[0].exception.description);
+                                        fail(unhandledExceptions[0].exception.message);


Oh, you figure it is the line breaks that break the protocol? Maybe we should still escape them here, in case the message also has multiple lines?

martinpitt · 2018-07-12T08:10:47Z

Still fails; I have an alternative approach locally, but I didn't work it out fully yet (higher priority things to do, sorry).

stefwalter · 2018-07-12T09:25:28Z

The driver is sending an empty JSON Object {}

stefwalter · 2018-07-12T12:45:28Z

Did a fixup which should fix the missing cases. Lets let this validate and then squash before merging.

martinpitt · 2018-07-12T12:46:38Z

test/common/cdp-driver.js

@@ -288,7 +299,19 @@ CDP.New(options)
                                pageLoadPromise = new Promise((resolve, reject) => { pageLoadResolve = resolve; pageLoadReject = reject; });

                            // run the command
-                            eval(command).then(success, fail);
+                            eval(command).then(reply => {
+                                var details, message;


this should be let

martinpitt · 2018-07-12T12:46:59Z

test/common/cdp-driver.js

@@ -72,6 +77,12 @@ function setupLogging(client) {
        resolveLogPromise();
    });

+    client.Runtime.exceptionThrown(info => {
+        var details = info.exceptionDetails;


this should be let

stefwalter · 2018-07-12T12:52:57Z

Fixed for review.

martinpitt · 2018-07-13T06:34:15Z

This works in general, thanks! I made a few TODOs at the top for particular crashes that this now picks up.

martinpitt · 2018-07-13T07:44:09Z

Unlike the serial console one, the test/verify/check-storage-nfs TestStoragePackages.testNfsMissingPackages failure reproduces well and reliably locally. Unfortunately the exception has no stack trace, and it does not show up on the JS console when manually clicking the "Install" button in the test case - it just shows the (expected) warning about fake-nfs-utils not being available from any repo.

So I wonder if that is actually from our injected test code:

-> wait: !ph_is_present("#dialog")
{"exceptionId":1,"text":"Uncaught (in promise)","lineNumber":0,"columnNumber":0,"scriptId":"1","url":"http://127.0.0.2:9091/cockpit/$c6f8515ba7f3c89612824b3b8f11b506ea999c43dae4dfd394db0dc4b1e55d82/storage/index.html#/","exception":{"type":"undefined"},"executionContextId":5}
<- raise {"type":"undefined"}

I'll add some debugging to ph_wait_cond().

martinpitt · 2018-07-13T11:05:42Z

I fixed the two issues above. Next move: bots.

martinpitt · 2018-07-19T15:44:47Z

There are three (really four, but the check-metrics test failures have the same root cause) remaining test failures. I have a local fix for check-machines, the other two still need investigation.

martinpitt · 2018-07-20T06:47:26Z

I now did a more general change to avoid the three new test failures, by filtering out unhandled exceptions from our own test suite. My TestMachines change from yesterday is still a nice cleanup, but I'll send it as a separate PR.

… dialog

Install a Chrome Devtools Protocol handler for unhandled exceptions, and fail the next CDP command with it. This got lost with the move from PhantomJS to Chrome. Some of our tests rely on failed ph_* function invocations, so we must not log failures of ph_wait_cond(); these are also technically unhandled exceptions. So turn this into a proper `PhWaitCondTimeout` error class, so that it can be identified reliably. Handle cases where undefined value is passed to fail/success in cdp-driver.js. Adjust the naughty override for the XTerm.js crash (cockpit-project#9641) as this now shows the actual underlying crash instead of just the followup element wait timeout. Closes cockpit-project#9639

martinpitt · 2018-07-21T09:15:35Z

The crash in testKubevirtMachinesList seems real, adding TODO item.

martinpitt · 2018-07-23T08:14:53Z

testKubevirtMachinesList crash: This happens while deleting a VM, and crashes in c3/c3. It seems the usage charts need to be stopped right before deleting a VM:

TypeError: Cannot read property 'data_types' of null
    at ChartInternal.c3_chart_internal_fn.isPieType (http://127.0.0.2:9091/cockpit/$a4bd17eb0dd459c340eb353b38f5eca269f0a7e1a87b8fa1362469fedf0dd530/kubernetes/kubernetes.js:130108:29)
    at ChartInternal.c3_chart_internal_fn.isArcType (http://127.0.0.2:9091/cockpit/$a4bd17eb0dd459c340eb353b38f5eca269f0a7e1a87b8fa1362469fedf0dd530/kubernetes/kubernetes.js:130119:22)
    at ChartInternal.c3_chart_internal_fn.getArc (http://127.0.0.2:9091/cockpit/$a4bd17eb0dd459c340eb353b38f5eca269f0a7e1a87b8fa1362469fedf0dd530/kubernetes/kubernetes.js:125358:31)
    at http://127.0.0.2:9091/cockpit/$a4bd17eb0dd459c340eb353b38f5eca269f0a7e1a87b8fa1362469fedf0dd530/kubernetes/kubernetes.js:125701:28
    at SVGPathElement.<anonymous> (http://127.0.0.2:9091/cockpit/$a4bd17eb0dd459c340eb353b38f5eca269f0a7e1a87b8fa1362469fedf0dd530/kubernetes/kubernetes.js:75525:34)
    at Object.tick [as c] (http://127.0.0.2:9091/cockpit/$a4bd17eb0dd459c340eb353b38f5eca269f0a7e1a87b8fa1362469fedf0dd530/kubernetes/kubernetes.js:75706:22)
    at d3_timer_mark (http://127.0.0.2:9091/cockpit/$a4bd17eb0dd459c340eb353b38f5eca269f0a7e1a87b8fa1362469fedf0dd530/kubernetes/kubernetes.js:68916:36)
    at d3_timer_step (http://127.0.0.2:9091/cockpit/$a4bd17eb0dd459c340eb353b38f5eca269f0a7e1a87b8fa1362469fedf0dd530/kubernetes/kubernetes.js:68897:16)

martinpitt · 2018-07-23T10:07:04Z

There are several other reports about similar errors in c3. This seems to be some race condition when switching pages, which I suppose also means "(un)render in React". If I disable the DonutChart in pkg/kubernetes/scripts/virtual-machines/components/VmMetricsTab.jsx, the crash does not occur, so it's definitively this component.

I also logged the podMetrics.cpu.usageNanoCores, used, and available values, and they are never invalid. E. g.

CPUChart usageNanoCores 129560717 used 13 available 87

I even tried to pin down const used = 20 to ensure it's not because of invalid data; the crash still happens. As this is outside of our control, does not really affect the user experience, and is already reported to c3, I think it's best to ignore this particular crash.

martinpitt · 2018-07-23T14:47:09Z

still fails, this needs to be generalized - it does not only fail in c3:

TypeError: Cannot read property 'data_types' of null
    at r.L.isPieType (http://127.0.0.2:9091/cockpit/$2cbb8f6e7523f0913b38bae2946d7d4f682ee68a565d8200743cd7ecbf2bcac1/kubernetes/kubernetes.js:56270:42)
    at r.L.isArcType (http://127.0.0.2:9091/cockpit/$2cbb8f6e7523f0913b38bae2946d7d4f682ee68a565d8200743cd7ecbf2bcac1/kubernetes/kubernetes.js:56278:25)
    at r.L.getArc (http://127.0.0.2:9091/cockpit/$2cbb8f6e7523f0913b38bae2946d7d4f682ee68a565d8200743cd7ecbf2bcac1/kubernetes/kubernetes.js:54177:30)
    at http://127.0.0.2:9091/cockpit/$2cbb8f6e7523f0913b38bae2946d7d4f682ee68a565d8200743cd7ecbf2bcac1/kubernetes/kubernetes.js:54303:47
    at SVGPathElement.<anonymous> (http://127.0.0.2:9091/cockpit/$2cbb8f6e7523f0913b38bae2946d7d4f682ee68a565d8200743cd7ecbf2bcac1/kubernetes/kubernetes.js:32170:42)
    at Object.s [as c] (http://127.0.0.2:9091/cockpit/$2cbb8f6e7523f0913b38bae2946d7d4f682ee68a565d8200743cd7ecbf2bcac1/kubernetes/kubernetes.js:29308:77)
    at Re (http://127.0.0.2:9091/cockpit/$2cbb8f6e7523f0913b38bae2946d7d4f682ee68a565d8200743cd7ecbf2bcac1/kubernetes/kubernetes.js:26664:65)
    at Ye (http://127.0.0.2:9091/cockpit/$2cbb8f6e7523f0913b38bae2946d7d4f682ee68a565d8200743cd7ecbf2bcac1/kubernetes/kubernetes.js:26659:21)

This gets triggered by the CPUUsage DonutChart in pkg/kubernetes/scripts/virtual-machines/components/VmMetricsTab.jsx. This seems to be some race condition when switching pages, which supposedly also means "(un)render in React". Cockpit code has no control over this, and it also does not break the user experience, so ignore the exception. See c3js/c3#2187 and simlilar issues bcbcarl/react-c3js#22 or c3js/c3#1205.

martinpitt · 2018-07-24T05:39:21Z

Yay! The remaining two failures on fedora-28 are well-known flakes, so retrying. This is ready for review now.

larskarlitski

Nice, thanks!

Install a Chrome Devtools Protocol handler for unhandled exceptions, and fail the next CDP command with it. This got lost with the move from PhantomJS to Chrome. Some of our tests rely on failed ph_* function invocations, so we must not log failures of ph_wait_cond(); these are also technically unhandled exceptions. So turn this into a proper `PhWaitCondTimeout` error class, so that it can be identified reliably. Handle cases where undefined value is passed to fail/success in cdp-driver.js. Adjust the naughty override for the XTerm.js crash (#9641) as this now shows the actual underlying crash instead of just the followup element wait timeout. Closes #9639

Install a Chrome Devtools Protocol handler for unhandled exceptions, and fail the next CDP command with it. This got lost with the move from PhantomJS to Chrome. Some of our tests rely on failed ph_* function invocations, so we must not log failures of ph_wait_cond(); these are also technically unhandled exceptions. So turn this into a proper `PhWaitCondTimeout` error class, so that it can be identified reliably. Handle cases where undefined value is passed to fail/success in cdp-driver.js. Adjust the naughty override for the XTerm.js crash (cockpit-project#9641) as this now shows the actual underlying crash instead of just the followup element wait timeout. Closes cockpit-project#9639 Cherry-picked from upstream master commit 9d3c17b, so that the naughty overrides match correctly.

Install a Chrome Devtools Protocol handler for unhandled exceptions, and fail the next CDP command with it. This got lost with the move from PhantomJS to Chrome. Some of our tests rely on failed ph_* function invocations, so we must not log failures of ph_wait_cond(); these are also technically unhandled exceptions. So turn this into a proper `PhWaitCondTimeout` error class, so that it can be identified reliably. Handle cases where undefined value is passed to fail/success in cdp-driver.js. Adjust the naughty override for the XTerm.js crash (#9641) as this now shows the actual underlying crash instead of just the followup element wait timeout. Closes #9639 Cherry-picked from upstream master commit 9d3c17b, so that the naughty overrides match correctly.

martinpitt mentioned this pull request Jul 11, 2018

Ensure we fail tests when a javascript exception happens #9539

Merged

1 task

martinpitt added the needswork label Jul 11, 2018

stefwalter reviewed Jul 11, 2018

View reviewed changes

martinpitt mentioned this pull request Jul 11, 2018

testSerialConsole fails on XTerm.js crash: Cannot read property 'dimensions' of undefined #9641

Closed

martinpitt commented Jul 12, 2018

View reviewed changes

stefwalter force-pushed the test-oopses branch from bebc82c to a099f55 Compare July 12, 2018 12:44

stefwalter removed the needswork label Jul 12, 2018

martinpitt commented Jul 12, 2018

View reviewed changes

stefwalter force-pushed the test-oopses branch from a099f55 to 1aa6bed Compare July 12, 2018 12:52

stefwalter force-pushed the test-oopses branch from 1aa6bed to 9190fd3 Compare July 12, 2018 13:56

martinpitt added the needswork label Jul 13, 2018

martinpitt force-pushed the test-oopses branch from 9190fd3 to 2ca9acc Compare July 13, 2018 11:05

martinpitt removed the needswork label Jul 13, 2018

cockpituous added the test-external label Jul 13, 2018

martinpitt force-pushed the test-oopses branch from 2ca9acc to d09eb57 Compare July 13, 2018 12:00

martinpitt added the needswork label Jul 19, 2018

martinpitt force-pushed the test-oopses branch from d09eb57 to 23e6f32 Compare July 20, 2018 06:47

martinpitt removed the needswork label Jul 20, 2018

martinpitt mentioned this pull request Jul 20, 2018

test: Robustify TestMachines.testCreate #9699

Closed

martinpitt added 2 commits July 21, 2018 11:12

storaged: Fix unhandled promise rejection when cancelling NFS install…

1a4d4ee

… dialog

martinpitt added the needswork label Jul 21, 2018

martinpitt mentioned this pull request Jul 23, 2018

[Pie Charts] Destroying chart while the chart is animating causes null error. c3js/c3#2187

Open

martinpitt force-pushed the test-oopses branch from 23e6f32 to 685ee4e Compare July 23, 2018 10:11

martinpitt added needswork and removed needswork labels Jul 23, 2018

martinpitt force-pushed the test-oopses branch from 685ee4e to 6fb1ba7 Compare July 23, 2018 14:48

martinpitt removed the needswork label Jul 23, 2018

martinpitt requested a review from stefwalter July 24, 2018 05:38

martinpitt requested review from allisonkarlitskaya and larskarlitski and removed request for allisonkarlitskaya July 24, 2018 07:31

mareklibra mentioned this pull request Jul 26, 2018

Switch to React from react-lite #9263

Merged

44 tasks

larskarlitski approved these changes Jul 27, 2018

View reviewed changes

larskarlitski merged commit 436bfe4 into cockpit-project:master Jul 27, 2018

martinpitt deleted the test-oopses branch July 27, 2018 06:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Fail CDP commands on unhandled exceptions #9639

test: Fail CDP commands on unhandled exceptions #9639

martinpitt commented Jul 11, 2018 •

edited

Loading

martinpitt commented Jul 11, 2018

stefwalter Jul 11, 2018

martinpitt Jul 11, 2018

stefwalter Jul 12, 2018

martinpitt Jul 12, 2018

martinpitt commented Jul 12, 2018

stefwalter commented Jul 12, 2018

stefwalter commented Jul 12, 2018

martinpitt Jul 12, 2018

martinpitt Jul 12, 2018

stefwalter commented Jul 12, 2018

martinpitt commented Jul 13, 2018

martinpitt commented Jul 13, 2018

martinpitt commented Jul 13, 2018

martinpitt commented Jul 19, 2018

martinpitt commented Jul 20, 2018

martinpitt commented Jul 21, 2018

martinpitt commented Jul 23, 2018

martinpitt commented Jul 23, 2018

martinpitt commented Jul 23, 2018

martinpitt commented Jul 24, 2018

larskarlitski left a comment

test: Fail CDP commands on unhandled exceptions #9639

test: Fail CDP commands on unhandled exceptions #9639

Conversation

martinpitt commented Jul 11, 2018 • edited Loading

martinpitt commented Jul 11, 2018

stefwalter Jul 11, 2018

Choose a reason for hiding this comment

martinpitt Jul 11, 2018

Choose a reason for hiding this comment

stefwalter Jul 12, 2018

Choose a reason for hiding this comment

martinpitt Jul 12, 2018

Choose a reason for hiding this comment

martinpitt commented Jul 12, 2018

stefwalter commented Jul 12, 2018

stefwalter commented Jul 12, 2018

martinpitt Jul 12, 2018

Choose a reason for hiding this comment

martinpitt Jul 12, 2018

Choose a reason for hiding this comment

stefwalter commented Jul 12, 2018

martinpitt commented Jul 13, 2018

martinpitt commented Jul 13, 2018

martinpitt commented Jul 13, 2018

martinpitt commented Jul 19, 2018

martinpitt commented Jul 20, 2018

martinpitt commented Jul 21, 2018

martinpitt commented Jul 23, 2018

martinpitt commented Jul 23, 2018

martinpitt commented Jul 23, 2018

martinpitt commented Jul 24, 2018

larskarlitski left a comment

Choose a reason for hiding this comment

martinpitt commented Jul 11, 2018 •

edited

Loading