Updated unit tests, Setup for Integration tests, and Code Coverage

[hitesh96db]

(copied from https://github.com/privly/privly-jetpack/pull/6#issuecomment-129990261 so that it's easy to find the explanation/procedure, will also add this in a .md)

Code Coverage for Jetpack

---

> Extracting the coverage info from an Instrumented CommonJS module

A coverage function is defined for every module(for which unit tests exist). This coverage function returns the coverage info for the corresponding module.

The coverage info exists in a variable in the generated instrumented module. We can get to know the variable name by following the same steps instanbul follows to generate the variable name.

To do this, we need the full file path and a hashing function. The hashing fuction is built using nsICryptoHash and the base64 implementation extracted from the node Crypto module(since istanbul uses it).

so the coverage function looks something like this --

var module = {
...
  coverage: function() {
    return eval(generateCoverageVarName("module.js"));
  },
...
}

Notice that a parameter module.js is passed to generateCoverageVarName. This is because the coverage name is a md5 sum + base64 encryption of the full file path. I couldn't find a way to get a pwd like path using Components(Privileged Javascript), so I've used a bash script to do that for me. This bash script(run_test.sh) is already being used to run tests on Travis-CI, so now we would use that to run tests locally as well. We can make good use of test.json here, which will now look like --

{
  "extension.privly.test": "true",
  "extension.privly.REPO_TOKEN": "replace_with_token",
  "extension.privly.extension_path": "replace_with_path",
}

So now, all we have to do is append the extension_path + "lib/module.js" to get the full file path, and then run the hashing function(md5 + b64) on it :)

Collecting the coverage info from every module

Once all the tests have run, we can loop over all the modules used in the tests and call the coverage function(if there exists one). Once we have the coverage info from all the files, we need to send it to a server that accepts this coverage info and generates a report, which is then sent to Coveralls.io.

NodeJS Server that accepts the coverage info and sends it to Coveralls.io

I've used Node.js to create the server. This is because generating the reports from the coverage json and sending those reports to Coverall.io is simple. To generate the reports, I've made use of the istanbul node API and to send the generated reports, I've used node-coveralls as recommended by Coveralls.io.

My plan was to deploy a server that accepts this coverage info. I've deployed it anyways here to test if it would work. But the problem is that the files being covered must also exist in the server, because while generating the report, the files are read using the path(of course, this path would no longer be valid in a deployed server) mentioned in the coverage info.

So, I guess the reports generated would work only when the server is run locally, i.e, on localhost. Maybe we can run the server locally on Travis-CI(?). Even if running the server locally works on Travis-CI, we need the Coveralls private repo token on Travis to send the data to Coveralls.io, which means the private token is no longer private. What can be done about this ?(We don't need to specify the token).

Code coverage posted using the above procedure - https://coveralls.io/github/hitesh96db/privly-jetpack :D.

[hitesh96db]

More....

the unit tests have been updated to use the options interface. Also, I've made a change in how the extension starts to run. Before the extension.js looked like this --

require("first_run").run();
require("posting_process").setup();
require("privly_ui").setup();
.
.
.
require("run_tests").run();

So the tests were run(if they were asked to), when the extension had everything setup, i.e, UI elements, content script instances, etc. But maybe it's better not to do anything before running the tests other than just opening the test loader and loading the modules to be tested. So now the entry point to run the extension is background_script.js which looks something like this --

var tests = require("test_runner");
if (testing) {
  tests.run()
} else {
  // runs the extension.
  require("extension.js").run();
}

[hitesh96db]

The run_test bash script does the following --

• Starts a NodeJS server.
• Checks if the env is Travis or Local, and sets the correct Firefox binary.
• Finds the pwd and replaces test.json with the correct current directory.
• Instruments all the modules in lib/ and saves them in a directory lib/instrument/
• Because the instrumented modules must exist in lib/ for the modules to work(because the require paths are hard coded), everything is copied from lib/instrument into lib/ with the extension .in.js - which means it's an instrumented file.
• Runs the tests using jpm run --prefs test.json
• test.json now includes the cwd.
• Once the tests are done, it removes the instrumented files from lib/
• Closes the server
• Sends the data to coveralls if the env is Travis.

[hitesh96db]

For any module to be covered, the module must include a coverage function which should look like this -- So, if the file is posting_process.js.

  .
  .
  coverage: function() { 
    var  cv = require("./coverage_var.js").coverageVar;
    return eval(cv.generate("posting_process.js"));
  },
  .
  .

and the XPCOM service should return the .in.js file --

   .
   .
   getPostingProcess: function() {
     return require("./posting_process.in.js");
   }
   .
   .

Why ? because the instrumented version holds the coverage information.

[hitesh96db]

Coverage. Build.

[irdan]

Unsurprisingly, the tests do not pass when istanbul is not installed. Either adding this to the testing documentation or making it part of the script seems like a good idea.

[irdan]

I wanted to note that when running the tests via the bash script, I get this output:

The following 1 file(s) had errors and were copied as-is
[ { file: 'xpcom.js', error: 'Line 29: Unexpected token this' } ]

However, all of the tests still run and pass.

[hitesh96db]

Yeah, that error shows up when you use istanbul instrument. It can be ignored.

[hitesh96db]

Because of the error, xpcom.js isn't instrumented but that's Ok.

[hitesh96db]

Let me know when you're done with a thorough review of this. I'll then add the necessary changes.

[irdan]

@smcgregor there are a few new files here that use eval. Do you know how much of an impact this will have on the mozilla review?

$ grep -r eval lib/*
lib/posting_process.js:    return eval(cv.generate("posting_process.js"));
lib/first_run.js:    return eval(cv.generate("first_run.js"));
lib/privly_ui.js:    return eval(cv.generate("privly_ui.js"));

FWIW, all of those lines are immediately preceded by

var cv = require("./coverage_var.js").coverageVar;

[irdan]

This is looking great. Thanks for making those changes.

I spoke with @smcgregor out of band and we will have to remove instances of eval when packaging wherever possible. Otherwise it will be a huge hurdle to overcome during the mozilla review process, not to mention any security drawbacks. Since eval is only being used to discover code coverage, it obviously doesn't need to be packaged in the extension.

I would purpose you find a way to not include eval when building the xpi for submission to mozilla. Perhaps abstracting the coverage function to a single file that can be added to .jpmignore? Or if that isn't feasible maybe making a wrapper around jpm xpi that removes the coverage functions from js files and then does a git checkout on the altered files after building.

[hitesh96db]

eval has to be present in the module, because each module has it's own instance of eval and other global objects like Array, Object, etc. So, an eval imported from a module A isn't the same as the eval object in module B. Code in module B -

  coverage: function() {
    // eval imported from another module.
    var another_eval = require("./eval.js").eval;
    console.log(another_eval !== eval);
    // will output true.
  },

So if the eval's are different, the env's in which they compile the strings are different.

Is it OK to place the eval in an if statement ?

  coverage: function() {
    if(!PACKAGED) {
      // use eval.
    }
  },

the check for a packaged extension would be similar to this.

Or Do we want eval to be totally wiped from the packaged extension code ? If so, I guess doing it manually would be the safest option.

[smcgregor]

eval has scope? I assumed eval is universally the global environment.

Removing it when packaging it for users is the best option, whether that is removing it piecewise from the modules or excluding their JS files (preferred).

[hitesh96db]

The global environment is specific to each module. We can't put the eval part in a separate JS file(that won't work) so I guess removing it piecewise would be the only option.

[hitesh96db]

Pushed a new commit. Removed the usage of eval.

[hitesh96db]

Latest Build and Coverage.

[hitesh96db]

Is this Ok to merge ?

[smcgregor]

Probably, I will spend more time with it this weekend.

[smcgregor]

I made a few minor comments inline. The most important part that needs changing is adding the @fileoverview directives. Once those are in, I will likely merge.

[smcgregor]

@irdan OK to merge?

[irdan]

@smcgregor All issues have been addressed. I'm :+1: to merge.