global-mode errors fired during mocha tests

[Spongman]

(current master)

i'm seeing a bunch of global mode binding errors being fired during the global-mode mocha tests. this is happening both in-browser and on the command line (including travis).

p5 had problems creating the global function "_initializeInstanceVariables", possibly because your code is already using that name as a variable. You may want to rename your variable to something else.
You just changed the value of "_initializeInstanceVariables", which was a p5 function. This could cause problems later if you're not careful.

[Zalastax]

This was a tricky bug to hunt down.
Those errors are generated when _createFriendlyGlobalFunctionBinder is called, which happens when sketch is not defined

p5.js/src/core/main.js

Lines 503 to 528 in 438b596

	if (!sketch) {
	this._isGlobal = true;
	p5.instance = this;
	// Loop through methods on the prototype and attach them to the window
	for (var p in p5.prototype) {
	if (typeof p5.prototype[p] === 'function') {
	var ev = p.substring(2);
	if (!this._events.hasOwnProperty(ev)) {
	if (Math.hasOwnProperty(p) && Math[p] === p5.prototype[p]) {
	// Multiple p5 methods are just native Math functions. These can be
	// called without any binding.
	friendlyBindGlobal(p, p5.prototype[p]);
	} else {
	friendlyBindGlobal(p, p5.prototype[p].bind(this));
	}
	}
	} else {
	friendlyBindGlobal(p, p5.prototype[p]);
	}
	}
	// Attach its properties to the window
	for (var p2 in this) {
	if (this.hasOwnProperty(p2)) {
	friendlyBindGlobal(p2, this[p2]);
	}
	}

Looking at the trace I found that this happens at

p5.js/src/core/init.js

Lines 17 to 29 in 438b596

	var _globalInit = function() {
	if (!window.mocha) {
	// If there is a setup or draw function on the window
	// then instantiate p5 in "global" mode
	if (
	((window.setup && typeof window.setup === 'function') ||
	(window.draw && typeof window.draw === 'function')) &&
	!p5.instance
	) {
	new p5();
	}
	}
	};

when window.mocha is undefined.
I found it strange that window.mocha was undefined when in a test but eventually noticed that some tests define an iframe which they run code in:

p5.js/test/unit/core/core.js

Lines 129 to 215 in 438b596

	suite('new p5() / global mode', function() {
	var P5_SCRIPT_URL = '../../lib/p5.js';
	var P5_SCRIPT_TAG = '<script src="' + P5_SCRIPT_URL + '"></script>';
	var iframe;
	function createP5Iframe(html) {
	html = html || P5_SCRIPT_TAG;
	iframe = document.createElement('iframe');
	document.body.appendChild(iframe);
	iframe.setAttribute('style', 'visibility: hidden');
	iframe.contentDocument.open();
	iframe.contentDocument.write(html);
	iframe.contentDocument.close();
	}
	teardown(function() {
	if (iframe) {
	iframe.parentNode.removeChild(iframe);
	iframe = null;
	}
	});
	test('is triggered when "setup" is in window', function() {
	return new Promise(function(resolve, reject) {
	createP5Iframe();
	iframe.contentWindow.setup = function() {
	resolve();
	};
	});
	});
	test('is triggered when "draw" is in window', function() {
	return new Promise(function(resolve, reject) {
	createP5Iframe();
	iframe.contentWindow.draw = function() {
	resolve();
	};
	});
	});
	test('works when p5.js is loaded asynchronously', function() {
	return new Promise(function(resolve, reject) {
	createP5Iframe();
	iframe.contentWindow.addEventListener(
	'load',
	function() {
	var win = iframe.contentWindow;
	win.setup = resolve;
	var script = win.document.createElement('script');
	script.setAttribute('src', P5_SCRIPT_URL);
	win.document.body.appendChild(script);
	},
	false
	);
	});
	});
	test('works on-demand', function() {
	return new Promise(function(resolve, reject) {
	createP5Iframe(
	[
	P5_SCRIPT_TAG,
	'<script>',
	'new p5();',
	'originalP5Instance = p5.instance',
	'myURL = p5.prototype.getURL();',
	'function setup() { setupCalled = true; }',
	'window.addEventListener("load", onDoneLoading, false);',
	'</script>'
	].join('\n')
	);
	iframe.contentWindow.onDoneLoading = resolve;
	}).then(function() {
	var win = iframe.contentWindow;
	assert.equal(typeof win.myURL, 'string');
	assert.strictEqual(win.setupCalled, true);
	assert.strictEqual(win.originalP5Instance, win.p5.instance);
	});
	});
	});

The problematic test is 'works when p5.js is loaded asynchronously' which accidentally adds a p5 script tag twice:

p5.js/test/unit/core/core.js

Line 174 in 438b596

createP5Iframe();

p5.js/test/unit/core/core.js

Lines 176 to 189 in 438b596

	iframe.contentWindow.addEventListener(
	'load',
	function() {
	var win = iframe.contentWindow;
	win.setup = resolve;
	var script = win.document.createElement('script');
	script.setAttribute('src', P5_SCRIPT_URL);
	win.document.body.appendChild(script);
	},
	false
	);

That test breaks if the iframe is created via: createP5Iframe(' '); (which causes it to not automatically inject a p5 script tag) but it does not break if these lines are removed:

p5.js/test/unit/core/core.js

Lines 183 to 186 in 438b596

	var script = win.document.createElement('script');
	script.setAttribute('src', P5_SCRIPT_URL);
	win.document.body.appendChild(script);

To me the load callback looks correct, but apparently it's not.
I would love if someone takes over and fixes the test.

There are many other things logged when running the tests and I think it would be great if we made sure that tests fail if they have: uncaught errors / lost promisses (even in iframes they create), if they create elements that are not cleaned up, or if they use console.log. I'd like tests to inject their own logger instance (possibly by simply shadowing window.console) and that the test asserts something about the log output. Tests that should log can then keep doing so (and they can assert that it actually happens) and tests that accidentally create warning logs will be detected.