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.

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

Karma is not able to run if any methods in Array.prototype are modified beforehand. #2671

Closed
AndyRightNow opened this issue Apr 20, 2017 · 5 comments · Fixed by karronoli/redpen#10 · May be fixed by Omrisnyk/npm-lockfiles#122, Omrisnyk/npm-lockfiles#132 or satoshinakamoto007/bitcore#1135

Comments

@AndyRightNow
Copy link

AndyRightNow commented Apr 20, 2017

Expected behaviour

Karma should be running.

Actual behaviour

Not able to parse the paths given in the config.files and throw an error:

ERROR [karma]: TypeError: Path must be a string. Received undefined
    at assertPath (path.js:7:11)
    at Object.extname (path.js:887:5)
    at d:\Test\test-karma\node_modules\.1.6.0@karma\lib\middleware\karma.js:181:32
    at d:\Test\test-karma\node_modules\.1.6.0@karma\lib\middleware\common.js:117:36
    at d:\Test\test-karma\node_modules\.4.1.11@graceful-fs\graceful-fs.js:78:16
    at FSReqWrap.readFileAfterClose [as oncomplete] (fs.js:439:3)

Environment Details

  • Karma version (output of karma --version): v1.6.0
  • Node.js version: v7.9.0
  • Relevant part of your karma.config.js file
module.exports = function (config) {
    config.set({
        browsers: ['PhantomJS'],
        files: [
            './test.js'
        ],
        singleRun: true,
        // coverage reporter generates the coverage
        reporters: ['progress'],
        plugins: [
            'karma-mocha',
            'karma-phantomjs-launcher'
        ]
    });
};

Steps to reproduce the behaviour

Run this snippet of script:

var Karma = require('karma').Server;
var path = require('path');

var fn = Array.prototype.indexOf;
delete Array.prototype['indexOf'];

Array.prototype['indexOf'] = function () {
    return fn.apply(this, arguments);
};

new Karma({
    configFile: path.resolve(__dirname, 'karma.conf.js')
}).start();

Cause

At this line in lib/middleware/karma.js:

for (var i in files.included) 

for...in is used to traverse an array and this is a known bad practice. Maybe there is a particular reason for using it here?

BenjaminVanRyseghem added a commit to BenjaminVanRyseghem/karma that referenced this issue Apr 27, 2017
Use `forEach` instead of `for..in` as it's better
practices and fix karma-runner#2671.
BenjaminVanRyseghem added a commit to BenjaminVanRyseghem/karma that referenced this issue Apr 28, 2017
Use `forEach` instead of `for..in` as it's better
practices and fix karma-runner#2671.
@wesleycho
Copy link
Member

Question - what are you trying to accomplish with modifying indexOf? The modification of the prototype method looks like a major code smell on first appearance.

There is a particular scenario that the for-in loop is accounting for I believe, so a solution is not as simple as replacing with forEach.

@AndyRightNow
Copy link
Author

@wesleycho In fact, I totally agree with you but that was written by someone else and I wasn't able to modify that at the time.

I am more like trying to figure out why for...in is used instead of trying to "correct" it. If there are some situations to tackle using this expression, is there any chance of using something like for...of or for (let i = 0; i < length; i++) to replace this while having no errors in those particular situations and the problem mentioned above?

@wesleycho
Copy link
Member

That will be possible when we start official work on 2.0 to remove node 0.10 and 0.12 support :) . We may be able to switch to for..of soon, just need the green light.

@wesleycho
Copy link
Member

As for for...in - I'm not sure. We could certainly switch to a more traditional for loop here I guess.

@dignifiedquire
Copy link
Member

I don't think there is a specific reason for using for...in, except that we don't want to rely on .forEach being present. So I suggest changing to for (var i = 0; i < arr.length; i++) which is still the safest way to iterate over arrays.

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