Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement ConsoleSandbox. Add the consoleMethCalled event. #1312

Merged
merged 9 commits into from
Sep 26, 2017
Merged

Implement ConsoleSandbox. Add the consoleMethCalled event. #1312

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1845fff
Implement ConsoleSandbox. Add the consoleMethCalled event.
AlexanderMoskovkin Sep 13, 2017
f87debf
Address remarks
AlexanderMoskovkin Sep 21, 2017
5102dce
Address Artem's remark
AlexanderMoskovkin Sep 22, 2017
cf706f2
Address Miher's remark
AlexanderMoskovkin Sep 22, 2017
8a32e28
Update test title
AlexanderMoskovkin Sep 22, 2017
f65f9b3
Reattach sandbox on the document cleaned event
AlexanderMoskovkin Sep 22, 2017
4266863
Send the consoleMethCalled method from iframes to the top window.
AlexanderMoskovkin Sep 26, 2017
8b679fc
Fix the test for IE
AlexanderMoskovkin Sep 26, 2017
d5afa2a
Comment the test for IE
AlexanderMoskovkin Sep 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/client/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@ class Hammerhead {
beforeXhrSend: this.sandbox.xhr.BEFORE_XHR_SEND_EVENT,
fetchSent: this.sandbox.fetch.FETCH_REQUEST_SENT_EVENT,
pageNavigationTriggered: this.pageNavigationWatch.PAGE_NAVIGATION_TRIGGERED_EVENT,
scriptElementAdded: this.sandbox.node.element.SCRIPT_ELEMENT_ADDED_EVENT
scriptElementAdded: this.sandbox.node.element.SCRIPT_ELEMENT_ADDED_EVENT,
consoleMethCalled: this.sandbox.console.CONSOLE_METH_CALLED
Copy link
Contributor

Choose a reason for hiding this comment

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

this.sandbox.console.CONSOLE_METH_CALLED -> this.sandbox.console.CONSOLE_METH_CALLED_EVENT

};

this.PROCESSING_COMMENTS = {
Expand Down Expand Up @@ -154,6 +155,9 @@ class Hammerhead {
case this.EVENTS.fetchSent:
return this.sandbox.fetch;

case this.EVENTS.consoleMethCalled:
return this.sandbox.console;

default:
return null;
}
Expand Down
27 changes: 27 additions & 0 deletions src/client/sandbox/console.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import SandboxBase from './base';

export default class ConsoleSandbox extends SandboxBase {
constructor () {
super();

this.CONSOLE_METH_CALLED = 'hammerhead|console|console-meth-called-event';
Copy link
Contributor

Choose a reason for hiding this comment

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

this.CONSOLE_METH_CALLED_EVENT = 'hammerhead|event|console-meth-called

}

attach (window) {
super.attach(window);

const sandbox = this;
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice catch


const proxyConsoleMeth = meth => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this function as private member of ConsoleSandbox, I think it is more redable.

sandbox.window.console[meth] = function () {
Copy link
Contributor

@miherlosev miherlosev Sep 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

In a new code we use the spread operator for an argument cloning
see
https://github.com/DevExpress/testcafe-hammerhead/blob/master/src/client/sandbox/node/document/index.js#L107
https://github.com/DevExpress/testcafe-hammerhead/blob/master/src/client/sandbox/node/window.js#L300
https://github.com/DevExpress/testcafe-hammerhead/blob/master/src/client/sandbox/event/index.js#L98

So, rewrite as

sandbox.window.console[meth] = (...args) => {
    sandbox.emit(sandbox.CONSOLE_METH_CALLED, { meth, args: args});
    sandbox.nativeMethods.consoleMeths[meth].apply(sandbox.nativeMethods.console, args);
}

sandbox.emit(sandbox.CONSOLE_METH_CALLED, { meth, args: arguments });
sandbox.nativeMethods.consoleMeths[meth].apply(sandbox.nativeMethods.console, arguments);
};
};

proxyConsoleMeth('log');
proxyConsoleMeth('info');
proxyConsoleMeth('error');
proxyConsoleMeth('warn');
}
}
3 changes: 3 additions & 0 deletions src/client/sandbox/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import XhrSandbox from './xhr';
import FetchSandbox from './fetch';
import StorageSandbox from './storages';
import ElectronSandbox from './electron';
import ConsoleSandbox from './console';
import { isIE, isWebKit, isElectron } from '../utils/browser';
import { create as createSandboxBackup, get as getSandboxBackup } from './backup';
import urlResolver from '../utils/url-resolver';
Expand Down Expand Up @@ -51,6 +52,7 @@ export default class Sandbox extends SandboxBase {
this.event = new EventSandbox(listeners, eventSimulator, elementEditingWatcher, unloadSandbox, messageSandbox, this.shadowUI, timersSandbox);
this.codeInstrumentation = new CodeInstrumentation(nodeMutation, this.event, this.cookie, this.upload, this.shadowUI, this.storageSandbox, liveNodeListFactory);
this.node = new NodeSandbox(nodeMutation, this.iframe, this.event, this.upload, this.shadowUI, liveNodeListFactory);
this.console = new ConsoleSandbox();

if (isElectron)
this.electron = new ElectronSandbox();
Expand Down Expand Up @@ -198,6 +200,7 @@ export default class Sandbox extends SandboxBase {
this.node.attach(window);
this.upload.attach(window);
this.cookie.attach(window);
this.console.attach(window);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we reattach ConsoleSandbox after document.write is called?
See reattach method and DOCUMENT_CLEANED_EVENT event.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems, we should


if (this.electron)
this.electron.attach(window);
Expand Down
10 changes: 10 additions & 0 deletions src/client/sandbox/native-methods.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,16 @@ class NativeMethods {
this.CSS2PropertyRemoveProperty = win.CSS2Property.prototype.removeProperty;
}

// Console
this.console = win.console;

this.consoleMeths = {
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't use an additional hierarchy level for similar cases.
See
https://github.com/DevExpress/testcafe-hammerhead/blob/master/src/client/sandbox/native-methods.js#L121
https://github.com/DevExpress/testcafe-hammerhead/blob/master/src/client/sandbox/native-methods.js#L176

Rewrite as

this.consoleLog = ...
this.consoleWarn = ...
etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? Is there a significant reason for this? I believe it's better to leave it as is to use

sandbox.nativeMethods.consoleMeths[meth]

instead of

const nativeMeth = console + meth[0].toUpperCase() + meth.slice(1);
sandbox.nativeMethods[nativeMeth];

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. If you use a console method as key then let you leave it as is.

log: win.console.log,
warn: win.console.warn,
error: win.console.error,
info: win.console.info
};

this.refreshClasses(win);
}

Expand Down
66 changes: 66 additions & 0 deletions test/client/fixtures/sandbox/console-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
var consoleSandbox = hammerhead.sandbox.console;

function argsToArray (args) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Array.prototype.slice.call ?

var res = [];
var i = 0;

while (i < args.length) {
res.push(args[i]);
i++;
}

return res;
}

test('consoleMethCalled event', function () {
var log = '';
var handledEvents = [];
var logFromEvents = '';

/* eslint-disable no-console */
var originMethods = {
log: console.log,
warn: console.warn,
error: console.error,
info: console.info
};

function addToLog (meth, args) {
log += argsToArray(args).join('');

originMethods[meth].apply(console, args);
}

window.console = {
Copy link
Contributor

Choose a reason for hiding this comment

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

To capture console trace you override methods that already overriden in sandbox.
Usually, we use an another way - we override the appropriate native methods.
See tests in https://github.com/DevExpress/testcafe-hammerhead/blob/master/test/client/fixtures/sandbox/node/methods-test.js
Use this way too.

log: function () {
addToLog('log', arguments);
},

warn: function () {
addToLog('warn', arguments);
},

error: function () {
addToLog('error', arguments);
},

info: function () {
addToLog('info', arguments);
}
};

consoleSandbox.on(consoleSandbox.CONSOLE_METH_CALLED, function (e) {
handledEvents.push(e.meth);
logFromEvents += argsToArray(e.args).join('');
});

window.console.log('1', '2');
window.console.warn('3', '4');
window.console.error('5', '6');
window.console.info('7', '8');
/* eslint-enable no-console */

deepEqual(handledEvents, ['log', 'warn', 'error', 'info']);
Copy link
Contributor

@miherlosev miherlosev Sep 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

handledConsoleMethodNames

deepEqual(log, '12345678');
deepEqual(logFromEvents, '12345678');
Copy link
Contributor

Choose a reason for hiding this comment

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

handledConsoleMethodArgs

});