Skip to content

use angular instead of global window#9743

Merged
Bargs merged 2 commits intoelastic:masterfrom
scampi:confirm-click-error
Jan 18, 2017
Merged

use angular instead of global window#9743
Bargs merged 2 commits intoelastic:masterfrom
scampi:confirm-click-error

Conversation

@scampi
Copy link
Contributor

@scampi scampi commented Jan 5, 2017

Chrome 55.0.2883 (Linux 0.0.0) confirmClick directive confirmed "before each" hook for "should trigger window.confirm when clicked" FAILED
	TypeError: Attempted to wrap confirm which is already stubbed
	    at checkWrappedMethod (eval at module.exports (http://localhost:5610/bundles/tests.bundle.js:151488:9), <anonymous>:1329:29)
	    at Object.wrapMethod (eval at module.exports (http://localhost:5610/bundles/tests.bundle.js:151488:9), <anonymous>:1366:21)
	    at Object.stub (eval at module.exports (http://localhost:5610/bundles/tests.bundle.js:151488:9), <anonymous>:3429:26)
	    at Context.<anonymous> (http://localhost:5610/bundles/tests.bundle.js:324823:39)
Chrome 55.0.2883 (Linux 0.0.0): Executed 447 of 2667 (1 FAILED) (skipped 6) (0 secs / 11.723 secChrome 55.0.2883 (Linux 0.0.0) confirmClick directive confirmed "after each" hook for "should trigger window.confirm when clicked" FAILED
	TypeError: window.confirm.restore is not a function
	    at Context.<anonymous> (http://localhost:5610/bundles/tests.bundle.js:324828:23)
Chrome 55.0.2883 (Linux 0.0.0): Executed 448 of 2667 (2 FAILED) (skipped 6) (0 secs / 11.723 secChrome 55.0.2883 (Linux 0.0.0) confirmClick directive not confirmed "before each" hook for "should not run the click function when canceled" FAILED
	TypeError: Attempted to wrap confirm which is already stubbed
	    at checkWrappedMethod (eval at module.exports (http://localhost:5610/bundles/tests.bundle.js:151488:9), <anonymous>:1329:29)
	    at Object.wrapMethod (eval at module.exports (http://localhost:5610/bundles/tests.bundle.js:151488:9), <anonymous>:1366:21)
	    at Object.stub (eval at module.exports (http://localhost:5610/bundles/tests.bundle.js:151488:9), <anonymous>:3429:26)
	    at Context.<anonymous> (http://localhost:5610/bundles/tests.bundle.js:324849:39)
Chrome 55.0.2883 (Linux 0.0.0): Executed 449 of 2667 (3 FAILED) (skipped 6) (0 secs / 11.752 secChrome 55.0.2883 (Linux 0.0.0) confirmClick directive not confirmed "after each" hook for "should not run the click function when canceled" FAILED
	TypeError: window.confirm.restore is not a function
	    at Context.<anonymous> (http://localhost:5610/bundles/tests.bundle.js:324854:23)

@elasticmachine
Copy link
Contributor

Can one of the admins verify this patch?

@thomasneirynck
Copy link
Contributor

jenkins, test this

@thomasneirynck thomasneirynck added review Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// labels Jan 6, 2017
@Bargs
Copy link
Contributor

Bargs commented Jan 6, 2017

@scampi mind if we close this since it turned out not to be the cause of our failures? Or do you think there's still a bug that needs fixing here?

@trevan
Copy link
Contributor

trevan commented Jan 7, 2017

I see these test failures in FF all the time and this PR fixes them.

@Bargs
Copy link
Contributor

Bargs commented Jan 7, 2017

Interesting! I'll take a closer look on monday

@Bargs Bargs self-assigned this Jan 7, 2017
@scampi
Copy link
Contributor Author

scampi commented Jan 10, 2017

@Bargs actually I keep on having this error, I got it again when running tests for #9809.
Anyway, it's better to use $window I think. Also, I don't know how much trust to put into that read-only claim but that'd be another reason to use $window.

Are you using chrome for running the tests ? I have version 55.0.2883.87 (64-bit).

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Bargs
Copy link
Contributor

Bargs commented Jan 17, 2017

jenkins, test this

@Bargs
Copy link
Contributor

Bargs commented Jan 17, 2017

@scampi could you rebase on master? I think that's all we need to get the tests passing. Otherwise LGTM

@spalger
Copy link
Contributor

spalger commented Jan 17, 2017

jenkins, test this

@Bargs Bargs self-requested a review January 18, 2017 15:54
@Bargs Bargs merged commit 6fd7280 into elastic:master Jan 18, 2017
Bargs pushed a commit that referenced this pull request Jan 18, 2017
@scampi scampi deleted the confirm-click-error branch January 18, 2017 23:24
@scampi
Copy link
Contributor Author

scampi commented Jan 18, 2017

thanks for the rebase @spalger I didn't read @Bargs' request before now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v5.3.0 v6.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

Comments