1160135 - Add tests to staticcacher

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Johan Lorenzo [:jlorenzo]]

Staticcacher is currently not covered by any tests. We can unit test it by stubbing sw-cache-helper. We can also add a couple of integration tests between Staticcacher and sw-cache-helper

Comment 1

[Johan Lorenzo [:jlorenzo]]

Attachment: PR

WIP

Comment 2

[Johan Lorenzo [:jlorenzo]]

I have a set of unit tests running. To check the integration with the Cache helper[1], I'd need to be able to use importScript() to import the NodeJS module? Do you see a way to do it, Salva?

[1] https://github.com/arcturus/sw-cache-helper

Comment 3

[Salvador de la Puente González [:salva]]

(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #2)
> I have a set of unit tests running. To check the integration with the Cache
> helper[1], I'd need to be able to use importScript() to import the NodeJS
> module? Do you see a way to do it, Salva?
> 
> [1] https://github.com/arcturus/sw-cache-helper

Oh. I see your point here. I realized Francisco used browserify to convert node modules into "browser" modules. I don't know if we can provide a general solution but let me find an specific one to workaround.

Comment 4

[Salvador de la Puente González [:salva]]

Well, the problem is as follows. `importScript()` runs the foreign code in the current context so all the global variables are automatically exposed in the service worker global scope. The problem is sw-cache-helper is using "strict mode" which checks undefined variables and only allow some well known globals to be directly accessed. For node, `module` is a well known global but for JS, it makes no sense... yet.

So, here is the solution (not tried): before using `importScript()`, at the initialization of the test, create a sw global object named module:

```js
self.module = {};

```

Now you should be able to use `importScript()` without throwing as module exists. The CacheHelper class should live inside `self.module.exports` now.

Hope it helps.

(It should be great if Bugzilla integrates Markdown by the way)

Comment 5

[Johan Lorenzo [:jlorenzo]]

I tried this solution, without success. Here's the error message I get:

> Chrome 44.0.2391 (Linux) Static cacher onInstall() should add the files to the cache FAILED
>	TypeError: Cannot read property 'getDefaultCache' of undefined

It doesn't seem like the Node module is correctly imported. What do you think, Salva?

Comment 6

[Johan Lorenzo [:jlorenzo]]

Btw, I also tried to add '/base/node_modules/sw-cache-helper/index.js' to SW_TESTS in sw-tests.js. It didn't help either.

Comment 7

[Salvador de la Puente González [:salva]]

Johan. Upload the code that is not working for you and add me to your repository as a collaborator and I will take a look. Thanks!

Comment 8

[Johan Lorenzo [:jlorenzo]]

I created the node-import-issue branch on my forked repo[1], and you should be now a collaborator of it.

[1] https://github.com/JohanLorenzo/serviceworkerware/tree/node-import-issue

Comment 9

[Salvador de la Puente González [:salva]]

Well, it's not the smartest way of do it [1], but it works. We can talk about how to improve the solution.

[1] https://github.com/JohanLorenzo/serviceworkerware/commit/fcaab08ccbe758b135e0f1521c6a426b9b8b7b24

Comment 10

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8599861 [details] [review]
PR

For the record, :francisco and :salva agreed on merging the cache helper back to SWW.

In this PR, I only did it for the staticcacher, as it was the only file that was impacted by this bug.

I don't have any issues on Chrome but I can't get the test executed on today's Firefox nightly. Karma keeps the tests in execution mode. Salva, how can I see what's wrong with it?

Comment 11

[Salvador de la Puente González [:salva]]

(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #10)
> Comment on attachment 8599861 [details] [review]
> PR
> 
> For the record, :francisco and :salva agreed on merging the cache helper
> back to SWW.
> 
> In this PR, I only did it for the staticcacher, as it was the only file that
> was impacted by this bug.
> 
> I don't have any issues on Chrome but I can't get the test executed on
> today's Firefox nightly. Karma keeps the tests in execution mode. Salva, how
> can I see what's wrong with it?

Johan, good work. Take a look at my comments on GitHub and ask for my review again. I will try to find the problems on Firefox when implementing bug #1165860.

Comment 12

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8599861 [details] [review]
PR

Thanks. It's now ready for another review.

Comment 13

[Salvador de la Puente González [:salva]]

Comment on attachment 8599861 [details] [review]
PR

Please, rebase and take into account this changes I introduce to ehance test isolation:
https://github.com/lodr/serviceworkerware/commit/5f3aff6f279d9b650e3b9a80a8c87b594899e8c0

Ask for my r once you're done but it's almost done.

Comment 14

[Johan Lorenzo [:jlorenzo]]

After applying the commit, I still have 8/44 test failures. I'll wait until bug 1165860 lands to clean up this patch.

Comment 15

[Salvador de la Puente González [:salva]]

Hey, Johan, bug 1166860 is done. Can you rebase? Thanks!

Comment 16

[Johan Lorenzo [:jlorenzo]]

Thanks for letting me know. I currently have 8 tests failing. I need to investigate deeper. I'll be working on them this week.

Comment 17

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8599861 [details] [review]
PR

Got the tests back passing. The changes due to the rebase are in the last commit.

Comment 18

[Salvador de la Puente González [:salva]]

Comment on attachment 8599861 [details] [review]
PR

Thank you Johan. Excellent work! You can merge.

Comment 19

[Salvador de la Puente González [:salva]]

master: 5d55085c08cbe5e87ef0f96d2bf9a63aab5041db

Comment 20

[Maria Angeles Oteo (:oteo)]

As NGA Program Manager suggested, let's replace the NGA-X milestones with FxOS-Sx ones (more generic ones), once Bug 1174794 has already landed