fix(watcher): use correct file sha in url query string

[chriscasola]

Use the most recently computed file sha present on the served file
when setting the file sha in the scripts tags added to the context.html.

This fixes an issue where the sha query arg would always be the sha of
the file when karma initially started and would not take into account
changes to the file when the watcher is running.

Closes #2317, #2264

---

I think this fixes both issues linked above. From what I could tell from my debugging, the sha on the file object in the served files array gets updated each time the file changes and the preprocessors run, but the sha on the included files does not. However for cache busting in the file URL path the sha from included was always used so the browser would end up requesting the original version of the file and it would be served by the browser cache.

I can add a test for this, but I wanted to get an idea of if this was an acceptable solution.

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

[googlebot]

CLAs look good, thanks!

[chriscasola]

Is there something I can do to work towards getting this merged? The travis CI build seems to be failing because of a lint error in the master branch.

[dignifiedquire]

Thank you, could you rebase onto master so CI is happy please?

[maksimr]

@chriscasola can you reproduce the problem with the test?
I think it will be great if we catch it with tests.

Thanks

[maksimr]

@chriscasola are you sure that this PR fixes the problem in the issue #2317?

Because as I see there a problem with "nocache"

Setting nocache to false it does : application.spec.js?a1b2c3d4f5g6
Setting nocache to true it does : application.spec.js?undefined <-----

#2317 (comment)

[maksimr]

Also, I am not sure that this PR fixes the issue #2264
because there a problem with preprocessing files when user set nocache

[chriscasola]

@maksimr -- I will work on adding a test to replicate the issue and I'll take another look at the issues I referenced.

The info in #2317 seems to indicate that in watch mode karma is not picking up changes to files, which is exactly the issue I was having that this PR fixes. I don't think what is described there is actually related to the "nocache" option but I will confirm.

As far as #2264 I think you're right that it is unrelated, I'll remove that reference from the commit message.

Thanks for your feedback!

[maksimr]

@chriscasola first of all thank you for your help.

About issue #2317, files with property doNotCache shouldn't have a sha(hash) at all. (IMHO)

[chriscasola]

@maksimr I'm not really sure what noCache should do in that case. According to this documentation it simply means karma should not cache the file contents in memory, and instead read from disk each time it is requested. I think that is separate from whether karma should prevent the browser from caching it or not.

I think the person who originally filed that issue was trying to use the noCache option because they noticed the same bug I did, which was that karma was rerunning tests on save, but not with the latest version of the test files.

[maksimr]

@chriscasola Well, Let's fix the problem and wait for the feedback

Thanks!

[chriscasola]

@maksimr -- I added a test and rebased

[dignifiedquire]

lgtm

[maksimr]

@chriscasola thanks

I had a look and I think that the current solution fixes the circumstances not the source of the problem.

We have a FileList class which managed files.
It contains property files. Each time when we try take a value of this property we call getter method. Getter always return new object.
Returned object contains two property served and included.
Served means that file should be served by the karma web server.
Included means that file should be included on the page by tag script.

Also, FileList has a method changeFile this method called by watcher when file changed.
Inside this method, we find a file by path passed from watcher after that we run preprocessors on the changed file.

Inside Preprocessor we set sha to the file after all preprocessors.

Ok, FileList waiting while preprocessor works when preprocessor finish work FileList trigger event file_list_modified and pass FileList.files as first argument.

Karma server and WebServer listen event file_list_modified.

Karma server on file_list_modifie emit event by socket to the browser and say browser restart tests.

WebServer on file_list_modifie event save link on files inside filesPromise.

After that browser mad request to the server for getting files. Here we go to karma middleware
which handles the request. It takes filePromise and get files from it.
For included fiels it generate scripts for served files it generate map which also passed to the client.

As you can see in karma middleware we have another request which uses included files for example '/context.json'.

So first, I didn't see where we can loose the sha and get the current problem.
Second I think if sha wrong it should be fixed in another place for example in Preprocessor or FileList becase we have code in different places which rely on sha from included file.

[chriscasola]

@maksimr I dug into this more and I think I see what is happening now.

In my project that is using karma but seeing the issue with changes not being picked up by the watch, we have the files property of our config set to something like:

[
  'src/**/*.js',
  'src/**/*.spec.js',
]

Obviously, it seems the second matcher there is redundant and can be removed to fix our issue, but because both of the matchers match files ending in .spec.js our test files end up appearing in the FileList under multiple patterns.

In changeFile the version of the file matched by the second matcher is found and preprocessed. Later on when the files getter is called, the second occurrence of the file, the one that was preprocessed, is put in the served list but the included list ends up pulling the first occurrence, which has the wrong sha and old file contents.

So in summary, I think the real bug here, is that if a file is matched by more than one file matcher from the karma config, changes are not picked up during the watch.

What would you recommend for a solution here? Ideas that come to mind are:

1. Show a warning if the file matchers match more than one file. This doesn't seem ideal since I don't think it makes sense to force people to write matchers that don't overlap.
2. Always use the file matched by the first matcher
3. Always use the file matched by the last matcher

[maksimr]

@chriscasola Thanks! This is very valuable information/research because now it sounds familiar.
I think we already had such issue and maybe still have.

#1499

What would you recommend for a solution here?

For me, we should have only one file which can be presented in several patterns/buckets.
I will look at the code.

And yeah, thanks again. Great job. 👍

P.S.
Could you create a repo which reproduces the problem If it's not a hassle?

[chriscasola]

@maksimr -- I think I see where the issue is and have a solution that ensures we always use the file from the first match. Just writing up a quick test and then I'll update this PR.

[maksimr]

Different things that maybe related to this problem:

raspo@353b9cf

#853

Allow exact file patterns to be specified below wildcard patterns
First pattern matching a file wins. It might be helpful to make an exception, if you specify exact file name (without any wildcard character).

[chriscasola]

@maksimr I just pushed what I think fixes the root of the problem, let me know what you think.

[chriscasola]

@maksimr @dignifiedquire can this be merged?

[dignifiedquire]

thank you

[lukaw3d]

This is a huge regression. There should be an option to disable it

All projects that have a configuration similar to below are broken after this change

files: [
  { pattern: 'src/**/*', included: false, served: true },
  { pattern: 'src/**/*.spec', included: true } // 232 tests
]

> Error 0 of 0 tests executed

and worse, projects with the following configuration are not even broken. It's just that most of their tests are not being run anymore

files: [
  { pattern: 'src/**/*', included: false, served: true },
  { pattern: 'src/**/*.spec', included: true }, // 232 tests
  { pattern: 'src2/**/*.spec', included: true } // 81 tests
]

> 81 of 81 tests executed

---

And this breaks plugins that append adapters to the end of the files list

files: [
  { pattern: 'src/**/*', included: false, served: true },
  { pattern: 'tests/**/*.spec', included: true },
  { pattern: 'node_modules/**/*', included: false, served: true }
  // <<< Plugin appends an adapter here. It is loaded from within node_modules
]