fix(file-list): Ensure duplicate files are handled properly

[m-a-r-c-e-l-i-n-o]

Previously, if file patterns amounted to duplicate files, the pattern resolution function would fail and Karma would continue on with an empty files list, namely "included". This led to confusing, generic adapter errors. This commit fixes that by replacing the offending code with a (likely) equivalent.
To replicate issue, include patterns that amount to duplicate files.
To see the error, wrap the following statement if (other && other.compare(p) < 0) return (contained inside file-list.js) in a try {} catch () {} block. In essence, other.compare does not exist.

Might be related to issue #2194

[m-a-r-c-e-l-i-n-o]

@dignifiedquire Sorry to pull you into this, I saw that you were involved in what could be a related issue. Would you happen to know what the intention of if (other && other.compare(p) < 0) return was suppose to be? As mentioned above, other.compare(p) does not appear to exist in the file object. I replaced it with byPath(other, p), but I'm not entirely sure that it's the way to go, especially since a few related e2e tests aren't passing.

[dignifiedquire]

Thank you for figuring this out, the commit introducing the commit was db42a7f and the .compare comes from here: https://github.com/karma-runner/karma/blob/master/lib/config.js#L42

Probably the issue is that other is not an instance of Pattern and that is the issue that needs fixing.

[m-a-r-c-e-l-i-n-o]

@dignifiedquire Thank you for the additional info, does normalizing the patterns before they are included in the lookup sound like a reasonable strategy to you? Something along the lines of:

if (p.constructor.name !== 'Pattern') {
   p = new Pattern(p.pattern, p.served, p.included, p.watched, p.nocache)
}
lookup[file.path] = p

[dignifiedquire]

I think we should make sure that here and here only pattern instances are passed in, so that we are sure that this._pattern is always and array of Pattern

[m-a-r-c-e-l-i-n-o]

@dignifiedquire I actually considered that, too. But, I don't fully understand the data flow in the system and thought that it would be risky to try to address the roots. However, if you're suggesting it, it makes me feel a bit more confident about that route — it seems to be more ideal. I thought about doing something along the lines of:

this._patterns = patterns.map(function (p) {
  if (p.constructor.name === 'Pattern') {
    return p
  }
  return new Pattern(p.pattern, p.served, p.included, p.watched, p.nocache)
})

Is that what you had in mind?

[dignifiedquire]

Sorry @m-a-r-c-e-l-i-n-o for not getting back on this earlier, I was away on a work trip and didn't have much time to look into this. It's important to get fixed properly so I will see that I can get you good answer by the end of today.

[dignifiedquire]

After reading your comment, yes I think this is the right way to go. Have you tested if the issues in #2194 are going away after a fix like this?

[m-a-r-c-e-l-i-n-o]

@dignifiedquire No worries. I just tested it and found two odd behaviors.

1. The patterns object will throw a "no adapter" error during unit tests if the object is replaced, so map is not usable here. Not a big deal, but would be nice to know why that is. You can try doing this._patterns = patterns.concat([]) to experience what I am talking about.
2. This and this do not appear to be roots of this. In my setup, the patterns object here is empty and here its full, so the error is still present since the patterns haven't been normalized. I'm assuming it's being populated by the plugins?

We need to understand this data flow a little better. Any thoughts?

[dignifiedquire]

Thanks a lot, turns out I have to fix it in that nasty location as the adapters might be editing things directly. See my PR for a fix, which I will merge and release as soon as CI is happy.