feat(config): Allow tests be to run in a new window instead of iframe #830

Merged
merged 1 commit into from Dec 7, 2013

Conversation

Projects
None yet
2 participants
@axemclion
Contributor

axemclion commented Nov 17, 2013

Some testing frameworks would need a new window and may not be able to run in an iframe. This pull request adds an option to open context.html in a new window instead of the iframe. For example, I am working on a (karma-telemetry)[http://github.com/axemclion/karma-telemetry] framework that uses telemetry to calculate rendering times of components. The times do not work in an iframe and need a new window.

Based on a discussion in groups - https://groups.google.com/forum/#!topic/karma-users/6Ud2TvwhESc

Note that to test this, the browsers need to enable popups. For chrome, this would be

 customLaunchers: {
        'ChromeNew' : {
            base: 'Chrome',
            flags: '--disable-popup-blocking'
        }
    },

@vojtajina

View changes

client/karma.js
@@ -42,7 +42,7 @@ var Karma = function(socket, context, navigator, location) {
};
contextWindow.onbeforeunload = function(e, b) {
- if (context.src !== 'about:blank') {
+ if (context[newWindow ? 'location' : 'src'] !== 'about:blank') {

This comment has been minimized.

@vojtajina

vojtajina Nov 22, 2013

Contributor

rather than this ternary if, can you create a function navigateContextTo (or something like that), which will handle it.

@vojtajina

vojtajina Nov 22, 2013

Contributor

rather than this ternary if, can you create a function navigateContextTo (or something like that), which will handle it.

@vojtajina

View changes

lib/config.js
@@ -193,6 +193,7 @@ var Config = function() {
this.usePolling = true;
this.reporters = ['progress'];
this.singleRun = false;
+ this.newWindow = false;

This comment has been minimized.

@vojtajina

vojtajina Nov 22, 2013

Contributor

This config should be client.x and instead of inlining the config value in the client.html, it will be passed to the client as the argument to execute event.

@vojtajina

vojtajina Nov 22, 2013

Contributor

This config should be client.x and instead of inlining the config value in the client.html, it will be passed to the client as the argument to execute event.

This comment has been minimized.

@vojtajina

vojtajina Nov 22, 2013

Contributor

I suggest useIframe (boolean), default to true. What do you think?

@vojtajina

vojtajina Nov 22, 2013

Contributor

I suggest useIframe (boolean), default to true. What do you think?

@vojtajina

View changes

lib/middleware/karma.js
+ });
+ }
+
+ // serve blank.html

This comment has been minimized.

@vojtajina

vojtajina Nov 22, 2013

Contributor

Remove? I don't think we need to serve blank.html

@vojtajina

vojtajina Nov 22, 2013

Contributor

Remove? I don't think we need to serve blank.html

@vojtajina

View changes

lib/middleware/karma.js
@@ -56,7 +56,14 @@ var createKarmaMiddleware = function(filesPromise, serveStaticFile,
// serve client.html
if (requestUrl === '/') {
- return serveStaticFile('/client.html', response);
+ return serveStaticFile('/client.html', response, function(data){

This comment has been minimized.

@vojtajina

vojtajina Nov 22, 2013

Contributor

As stated above, I would rather pass the useIframe config as a part of the client config.

@vojtajina

vojtajina Nov 22, 2013

Contributor

As stated above, I would rather pass the useIframe config as a part of the client config.

@vojtajina

View changes

static/client.html
- <iframe id="context" src="about:blank" width="100%" height="100%"></iframe>
+ <script type = 'text/javascript'>
+ var newWindow = ('%CONTEXT_NEW_WINDOW%' === 'true');
+ if (newWindow){

This comment has been minimized.

@vojtajina

vojtajina Nov 22, 2013

Contributor

This code should be in client/karma.js rather than inlined here. Feel free to keep the <iframe> here in the template. And the client Karma object will decide based on the config (passed to execute) - if useIframe is false, then it will do window.open(), otherwise use the iframe. If there is no window, create one (so that even if I close the popup window and then Karma tries to run, it will re-create the window).

@vojtajina

vojtajina Nov 22, 2013

Contributor

This code should be in client/karma.js rather than inlined here. Feel free to keep the <iframe> here in the template. And the client Karma object will decide based on the config (passed to execute) - if useIframe is false, then it will do window.open(), otherwise use the iframe. If there is no window, create one (so that even if I close the popup window and then Karma tries to run, it will re-create the window).

@vojtajina

This comment has been minimized.

Show comment
Hide comment
@vojtajina

vojtajina Nov 22, 2013

Contributor

@axemclion This is awesome! I left many comments, let me know what you think and if it makes any sense to you...

I'm thinking, we should add --disable-popup to Chrome's default flags. It won't hurt and will make it easier to use with this "new window" feature.

Contributor

vojtajina commented Nov 22, 2013

@axemclion This is awesome! I left many comments, let me know what you think and if it makes any sense to you...

I'm thinking, we should add --disable-popup to Chrome's default flags. It won't hurt and will make it easier to use with this "new window" feature.

@axemclion

View changes

client/karma.js
@@ -4,7 +4,7 @@ var util = require('./util');
/* jshint unused: false */
-var Karma = function(socket, context, navigator, location) {
+var Karma = function(socket, navigator, location) {

This comment has been minimized.

@axemclion

axemclion Nov 26, 2013

Contributor

Removed context as no one else is using this. This is instantiated the first time the context URL is set.

@axemclion

axemclion Nov 26, 2013

Contributor

Removed context as no one else is using this. This is instantiated the first time the context URL is set.

@axemclion

View changes

client/karma.js
@@ -19,6 +19,24 @@ var Karma = function(socket, context, navigator, location) {
this.VERSION = constant.VERSION;
this.config = {};
+ var context = null;
+ var contextUrl = function(url) {

This comment has been minimized.

@axemclion

axemclion Nov 26, 2013

Contributor

Wanted to get a better name of this - any suggestions ?

@axemclion

axemclion Nov 26, 2013

Contributor

Wanted to get a better name of this - any suggestions ?

This comment has been minimized.

@vojtajina

vojtajina Nov 27, 2013

Contributor

Well, it does many things... that's why it's hard to give it a name...

@vojtajina

vojtajina Nov 27, 2013

Contributor

Well, it does many things... that's why it's hard to give it a name...

This comment has been minimized.

@vojtajina

vojtajina Nov 27, 2013

Contributor

If we do the refactoring I mentioned below, you can get rid off the "getter" functionality (returning the current src value) and call it navigateContextTo or something...

@vojtajina

vojtajina Nov 27, 2013

Contributor

If we do the refactoring I mentioned below, you can get rid off the "getter" functionality (returning the current src value) and call it navigateContextTo or something...

@axemclion

View changes

lib/executor.js
@@ -23,6 +23,7 @@ var Executor = function(capturedBrowsers, config, emitter) {
pendingCount = capturedBrowsers.length;
runningBrowsers = capturedBrowsers.clone();
emitter.emit('run_start', runningBrowsers);
+ config.client.useIframe = config.useIframe;

This comment has been minimized.

@axemclion

axemclion Nov 26, 2013

Contributor

@vojtajina Is this what you meant when you said pass it as client.x ? Kind of seems odd to have a config.client, and assigning another property to it

@axemclion

axemclion Nov 26, 2013

Contributor

@vojtajina Is this what you meant when you said pass it as client.x ? Kind of seems odd to have a config.client, and assigning another property to it

This comment has been minimized.

@vojtajina

vojtajina Nov 27, 2013

Contributor

Almost. There is already "config.client" that we pass into the client.

So you can add the default client.useIframe = true into https://github.com/karma-runner/karma/blob/master/lib/config.js#L206 and document it as config.client.useIframe.
Does it make sense?

@vojtajina

vojtajina Nov 27, 2013

Contributor

Almost. There is already "config.client" that we pass into the client.

So you can add the default client.useIframe = true into https://github.com/karma-runner/karma/blob/master/lib/config.js#L206 and document it as config.client.useIframe.
Does it make sense?

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Nov 26, 2013

Contributor

@vojtajina Made the fixes you suggested. How do these look ?

Contributor

axemclion commented Nov 26, 2013

@vojtajina Made the fixes you suggested. How do these look ?

@vojtajina

View changes

client/karma.js
+ contextWindow.__karma__.error('Some of your tests did a full page reload!');
+ }
+ } else {
+ // In case of a new window, there is no src attribute, and the location changes only after the page loads

This comment has been minimized.

@vojtajina

vojtajina Nov 27, 2013

Contributor

You know what. How about if we refactor this - instead of relying on the context.src, add another state variable (similar to hasError or startEmitted), say reloadingContext. This will be set to false when execution starting (when we reload the context to start test execution). Then, before we clear the context, we set it to true. Because essentially this check is here, so that we ignore the context refresh at the end of the tests (when clearing stuff back to about:blank). What do you think?

@vojtajina

vojtajina Nov 27, 2013

Contributor

You know what. How about if we refactor this - instead of relying on the context.src, add another state variable (similar to hasError or startEmitted), say reloadingContext. This will be set to false when execution starting (when we reload the context to start test execution). Then, before we clear the context, we set it to true. Because essentially this check is here, so that we ignore the context refresh at the end of the tests (when clearing stuff back to about:blank). What do you think?

@vojtajina

View changes

config.tpl.js
+ singleRun: false,
+
+ // Runs tests in an iFrame instead of opening them in a new window
+ useIframe : true

This comment has been minimized.

@vojtajina

vojtajina Nov 27, 2013

Contributor

You don't even have to add it here... (the default config is in lib/config.js) - this is the template used by karma init.

@vojtajina

vojtajina Nov 27, 2013

Contributor

You don't even have to add it here... (the default config is in lib/config.js) - this is the template used by karma init.

@vojtajina

View changes

static/context.html
@@ -16,7 +16,12 @@
manner. -->
<script type="text/javascript">
// sets window.__karma__ and overrides console and error handling
- window.parent.karma.setupContext(window);
+ // Use window.opener if this was opened by someone else - in a new window
+ if (window.opener){

This comment has been minimized.

@vojtajina

vojtajina Nov 27, 2013

Contributor

style: can you add a space ) {?

@vojtajina

vojtajina Nov 27, 2013

Contributor

style: can you add a space ) {?

@vojtajina

This comment has been minimized.

Show comment
Hide comment
@vojtajina

vojtajina Nov 27, 2013

Contributor

@axemclion Nice! I added some more comments. Can you also make sure the tests are passing? (https://travis-ci.org/karma-runner/karma/jobs/14531746) YOu can run them locally with grunt test:client.

Can you also add tests for this new functionality in the client code?

  • Test that it opens new window if somebody closed the window from previous run.
  • Test that it opens new window or uses the iframe based on the configuration.
Contributor

vojtajina commented Nov 27, 2013

@axemclion Nice! I added some more comments. Can you also make sure the tests are passing? (https://travis-ci.org/karma-runner/karma/jobs/14531746) YOu can run them locally with grunt test:client.

Can you also add tests for this new functionality in the client code?

  • Test that it opens new window if somebody closed the window from previous run.
  • Test that it opens new window or uses the iframe based on the configuration.
@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Nov 27, 2013

Contributor

Made the changes - how do these look. Also, any advice on where to write the test for the useIframe flag ?

Contributor

axemclion commented Nov 27, 2013

Made the changes - how do these look. Also, any advice on where to write the test for the useIframe flag ?

@vojtajina

View changes

client/karma.js
+ }
+ }
+
+ if (typeof url === 'undefined') {

This comment has been minimized.

@vojtajina

vojtajina Nov 29, 2013

Contributor

You never using this function as a getter, so you can remove this.

@vojtajina

vojtajina Nov 29, 2013

Contributor

You never using this function as a getter, so you can remove this.

@vojtajina

View changes

client/karma.js
- if (context.src !== 'about:blank') {
- // TODO(vojta): show what test (with explanation about jasmine.UPDATE_INTERVAL)
- contextWindow.__karma__.error('Some of your tests did a full page reload!');
+ if (reloadingContext) {

This comment has been minimized.

@vojtajina

vojtajina Nov 29, 2013

Contributor

This condition is wrong, it should be !reloadingContext

@vojtajina

vojtajina Nov 29, 2013

Contributor

This condition is wrong, it should be !reloadingContext

This comment has been minimized.

@vojtajina

vojtajina Nov 29, 2013

Contributor

reloadingContext true means, Karma is reloading the context (after a test run) and therefore it's fine.

@vojtajina

vojtajina Nov 29, 2013

Contributor

reloadingContext true means, Karma is reloading the context (after a test run) and therefore it's fine.

@vojtajina

View changes

docs/config/01-configuration-file.md
+
+**Default:** `true`
+
+**CLI:** `--use-iframe`, `no-use-iframe`

This comment has been minimized.

@vojtajina

vojtajina Nov 29, 2013

Contributor

I don't think we need a CLI option for this.

@vojtajina

vojtajina Nov 29, 2013

Contributor

I don't think we need a CLI option for this.

@vojtajina

View changes

lib/cli.js
@@ -39,6 +39,12 @@ var processArgs = function(argv, options, fs, path) {
options.singleRun = options.singleRun === 'true';
}
+ if (options.useIframe === false) {

This comment has been minimized.

@vojtajina

vojtajina Nov 29, 2013

Contributor

As above, I would remove this. I don't think we need a CLI option for this.

@vojtajina

vojtajina Nov 29, 2013

Contributor

As above, I would remove this. I don't think we need a CLI option for this.

@vojtajina

This comment has been minimized.

Show comment
Hide comment
@vojtajina

vojtajina Nov 29, 2013

Contributor

@axemclion Thanks, I sent you some more comments.

Also, please make sure the Travis build is green. Currently the build is failing because of a syntax error, which means you even didn't try to run Karma with your changes ;-)

Contributor

vojtajina commented Nov 29, 2013

@axemclion Thanks, I sent you some more comments.

Also, please make sure the Travis build is green. Currently the build is failing because of a syntax error, which means you even didn't try to run Karma with your changes ;-)

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Nov 30, 2013

Contributor

@vojtajina Ran the tests, but was always seeing failures on Windows. My bad.

I now see an error in Travis. Will wait for that to be fixed before my pull request.

Contributor

axemclion commented Nov 30, 2013

@vojtajina Ran the tests, but was always seeing failures on Windows. My bad.

I now see an error in Travis. Will wait for that to be fixed before my pull request.

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Nov 30, 2013

Contributor

Not sure if you did a force push, but I am missing a commit from Nov 28 about caching files. It was there on my branch yesterday. Anyway, rebased it to the latest from your repo.

Contributor

axemclion commented Nov 30, 2013

Not sure if you did a force push, but I am missing a commit from Nov 28 about caching files. It was there on my branch yesterday. Anyway, rebased it to the latest from your repo.

@vojtajina

This comment has been minimized.

Show comment
Hide comment
@vojtajina

vojtajina Nov 30, 2013

Contributor

Oh, you are on Windows.... good luck ;-)

@axemclion sorry, I did remove that commit, as it was failing the build and needs more work. You can do git rebase -i HEAD~10 and delete that commit from your branch...

Regards the test - there is a "client" test suite (run by Karma), these tests are in test/client. So test different scenarios - with useIframe: true, useIframe: false and check that the Karma client object does the correct thing (get the iframe element / create popup window). You will probably want to mock out the window.open API.

Contributor

vojtajina commented Nov 30, 2013

Oh, you are on Windows.... good luck ;-)

@axemclion sorry, I did remove that commit, as it was failing the build and needs more work. You can do git rebase -i HEAD~10 and delete that commit from your branch...

Regards the test - there is a "client" test suite (run by Karma), these tests are in test/client. So test different scenarios - with useIframe: true, useIframe: false and check that the Karma client object does the correct thing (get the iframe element / create popup window). You will probably want to mock out the window.open API.

@vojtajina

View changes

client/karma.js
@@ -42,8 +57,7 @@ var Karma = function(socket, context, navigator, location) {
};
contextWindow.onbeforeunload = function(e, b) {
- if (context.src !== 'about:blank') {
- // TODO(vojta): show what test (with explanation about jasmine.UPDATE_INTERVAL)

This comment has been minimized.

@vojtajina

vojtajina Nov 30, 2013

Contributor

keep this todo for me please

@vojtajina

vojtajina Nov 30, 2013

Contributor

keep this todo for me please

@vojtajina

View changes

docs/config/01-configuration-file.md
@@ -311,6 +311,15 @@ on whether all tests passed or any tests failed.
is handed off to [socket.io](https://github.com/LearnBoost/Socket.IO/wiki/Configuring-Socket.IO) (which manages the communication
between browsers and the testing server).
+## useIframe

This comment has been minimized.

@vojtajina

vojtajina Nov 30, 2013

Contributor

this should be called client.useIframe

@vojtajina

vojtajina Nov 30, 2013

Contributor

this should be called client.useIframe

@axemclion

View changes

client/karma.js
+ context = window.open('about:blank');
+ }
+ }
+ if (context){

This comment has been minimized.

@axemclion

axemclion Dec 1, 2013

Contributor

I needed to add this line as in the karma.spec.js, the mocks for window.open and document.getElementById did not work when clearContext is being called.

@axemclion

axemclion Dec 1, 2013

Contributor

I needed to add this line as in the karma.spec.js, the mocks for window.open and document.getElementById did not work when clearContext is being called.

This comment has been minimized.

@vojtajina

vojtajina Dec 4, 2013

Contributor

nope, this condition is here only because of the test, let's rather fix the test.

In the test, create a mock that returns a mock context (window or element) and then you can remove this condition and even assert that correct property was set (src if it's an iframe and location if it's a popup window).

@vojtajina

vojtajina Dec 4, 2013

Contributor

nope, this condition is here only because of the test, let's rather fix the test.

In the test, create a mock that returns a mock context (window or element) and then you can remove this condition and even assert that correct property was set (src if it's an iframe and location if it's a popup window).

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Dec 1, 2013

Contributor

I ran grunt test:client successfully, and also added tests. However, I still see that travis is broken, but for coverage e2e tests.

Contributor

axemclion commented Dec 1, 2013

I ran grunt test:client successfully, and also added tests. However, I still see that travis is broken, but for coverage e2e tests.

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Dec 3, 2013

Contributor

Rebased the code again on the latest

Contributor

axemclion commented Dec 3, 2013

Rebased the code again on the latest

@axemclion

This comment has been minimized.

Show comment
Hide comment
@vojtajina

View changes

client/karma.js
+ var context = null;
+ var navigateContextTo = function(url) {
+ if (context === null || (self.config.useIframe === false && context.closed === true)) {
+ // context has not yet been setup. Need to set up up once

This comment has been minimized.

@vojtajina

vojtajina Dec 4, 2013

Contributor

just typo in the comment "set up one."

@vojtajina

vojtajina Dec 4, 2013

Contributor

just typo in the comment "set up one."

@vojtajina

View changes

client/main.js
@@ -17,5 +17,4 @@ var socket = io.connect('http://' + location.host, {
// instantiate the updater of the view
new StatusUpdater(socket, util.elm('title'), util.elm('banner'), util.elm('browsers'));
-
-window.karma = new Karma(socket, util.elm('context'), window.navigator, window.location);
+window.karma = new Karma(socket, window.navigator, window.location);

This comment has been minimized.

@vojtajina

vojtajina Dec 4, 2013

Contributor

to make testing simple, you should pass in the iframe element (util.elm('context')) and window.open. Then it's gonna be much simpler to pass in fake versions in the test... and you will be able to remove the condition from #830

@vojtajina

vojtajina Dec 4, 2013

Contributor

to make testing simple, you should pass in the iframe element (util.elm('context')) and window.open. Then it's gonna be much simpler to pass in fake versions in the test... and you will be able to remove the condition from #830

This comment has been minimized.

@axemclion

axemclion Dec 4, 2013

Contributor

do you mean change the signature of new Karma to something like

window.karma = new Karma(socket, util.elm('context'), window.open, window.navigator, window.location);

@axemclion

axemclion Dec 4, 2013

Contributor

do you mean change the signature of new Karma to something like

window.karma = new Karma(socket, util.elm('context'), window.open, window.navigator, window.location);

This comment has been minimized.

@vojtajina

vojtajina Dec 4, 2013

Contributor

yep

@vojtajina

vojtajina Dec 4, 2013

Contributor

yep

This comment has been minimized.

@axemclion

axemclion Dec 4, 2013

Contributor

I was not very comfortable adding one more arg to the function.

The problem with the earlier approach was that the mock for window.open or document.getElementById no longer exists inside a setTimeout call.

@axemclion

axemclion Dec 4, 2013

Contributor

I was not very comfortable adding one more arg to the function.

The problem with the earlier approach was that the mock for window.open or document.getElementById no longer exists inside a setTimeout call.

@vojtajina

View changes

docs/config/01-configuration-file.md
+
+**Default:** `true`
+
+**Description:** Opens the test case in a new iFrame

This comment has been minimized.

@vojtajina

vojtajina Dec 4, 2013

Contributor

"Run the tests inside an iframe or a new window."

@vojtajina

vojtajina Dec 4, 2013

Contributor

"Run the tests inside an iframe or a new window."

@vojtajina

View changes

docs/config/01-configuration-file.md
+
+**Description:** Opens the test case in a new iFrame
+
+If `true`, Karma open the tests in an iFrame. If false, the test cases are opened in a new window. Some tests may not run in an iFrame and may need a new window to run.

This comment has been minimized.

@vojtajina

vojtajina Dec 4, 2013

Contributor

If true, Karma runs the tests inside an iframe. If false, Karma runs the tests in a new window.

@vojtajina

vojtajina Dec 4, 2013

Contributor

If true, Karma runs the tests inside an iframe. If false, Karma runs the tests in a new window.

-
-window.karma = new Karma(socket, util.elm('context'), window.navigator, window.location);
+window.karma = new Karma(socket, util.elm('context'), window.open,
+ window.navigator, window.location);

This comment has been minimized.

@axemclion

axemclion Dec 4, 2013

Contributor

line break for JSHint to be happy.

@axemclion

axemclion Dec 4, 2013

Contributor

line break for JSHint to be happy.

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Dec 5, 2013

Contributor

Changes done, travis passing.

Contributor

axemclion commented Dec 5, 2013

Changes done, travis passing.

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Dec 6, 2013

Contributor

Any more code review changes, or are we good with the changes so far ?

Contributor

axemclion commented Dec 6, 2013

Any more code review changes, or are we good with the changes so far ?

vojtajina added a commit that referenced this pull request Dec 7, 2013

Merge pull request #830 from axemclion/master
feat(config): Allow tests be to run in a new window instead of iframe

@vojtajina vojtajina merged commit 9f0abfd into karma-runner:master Dec 7, 2013

@vojtajina

This comment has been minimized.

Show comment
Hide comment
@vojtajina

vojtajina Dec 7, 2013

Contributor

Thanks @axemclion !!!

It's merged. Did you sign the CLA please?

Also, I updated the Jasmine adapter (karma-runner/karma-jasmine@ead8a77), but we still need to handle "disabling the pop-up windows" better, could you look into #849?

Contributor

vojtajina commented Dec 7, 2013

Thanks @axemclion !!!

It's merged. Did you sign the CLA please?

Also, I updated the Jasmine adapter (karma-runner/karma-jasmine@ead8a77), but we still need to handle "disabling the pop-up windows" better, could you look into #849?

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Dec 7, 2013

Contributor

Awesome, thank you. I will look at #849. I am currently working on fixing the grunt test errors on windows.

Contributor

axemclion commented Dec 7, 2013

Awesome, thank you. I will look at #849. I am currently working on fixing the grunt test errors on windows.

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