Proposal for integration/unit testing

[geoffdutton]

@gorhill I'm not sure if this is something you're interested in, but here is my proposal for unit testing: https://github.com/geoffdutton/uBlock/tree/unit-tests-proposal
Here is the current diff: https://github.com/gorhill/uBlock/compare/master...geoffdutton:unit-tests-proposal?expand=1

I've written a few test cases, and would be happy to contribute more. I think it can help for two main reasons:

1. Preventing regressions
2. Refactoring code and trying performance improvements

Number 2 is especially nice, such as when parsing new filter syntaxes or running extreme scenarios.

I'd be happy to go into more detail on how to write tests, when to mock functions, how to mimic browser features, etc. I chose jest because it's fast, and you can mimic any browser like here.

Let me know if this is something you're interested in incorporating.

[jspenguin2017]

My 2 cents:
Mocking chrome namespace is pretty difficult, have you considered Puppeteer? You probably would need Puppeteer for integration test anyway.

// I can't type µBlock...

Same.

[gorhill]

Yes, I'm interested in this, thanks for starting this. Now, this is all new to me, so I need to find my way into all this. I git cloned your repo, and first thing is, how do I launch tests?

// I can't type µBlock...

I use right-Alt + m, though maybe this works on my side because I use US intl keyboard with dead keys.

I saw that the build script needs rm -r $DES/js/__tests__, isn't possible to not copy __tests__ in the first place by placing it outside $DES/js/?

[jspenguin2017]

how do I launch tests?

First install Node.js:

curl -sL https://deb.nodesource.com/setup_8.x | sudo -E bash -
sudo apt-get install -y nodejs

Then install the 591 required dependencies:

npm install

Then execute the test script:

npm test

I use right-Alt + m

That doesn't do anything on mine, I think that doesn't work for most people.

[gorhill]

That doesn't do anything on mine, I think that doesn't work for most people.

I could create an alias of µBlock => uBlock, with the long term goal of phasing out usage of µBlock.

[geoffdutton]

@jspenguin2017 I've used Puppeteer, it's great. I would consider it's best use case for more "end-to-end" tests. I've found it easier to have sort of a blank slate and mimic certain browser features. I will commit some more tests with further examples of mocking out the chrome object. Mocking it out is not perfect because you're hoping the apis do what they say they do, and then you get to the real world with a quirky api that doesn't quite work as you thought. Just curious, can you think of an example where mocking the chrome object was an issue?

Here's my thought process as I was trying to figure out what this commit was fixing. Since it's just modifying the regex, it's not really necessary to run that in a full chrome environment, I'd rather isolate the specific function µBlock.compileFilters. So I wrote out sort of a dummy test by first just making a simple assertion:

const result = ub.compileFilters(stevenBlackRaw);
expect(result).toBe('');

and then fired up the tests with npm run tdd. Since I mocked out µBlock.staticNetFilteringEngine.compile, I have access to the full call history via the .mock property. So I changed the regex to what it was before the above commit, and then did something like:

const result = ub.compileFilters(stevenBlackRaw);
// .mock.calls is an array of calls in the order they happened,
// and each array entry is an array of the arguments passed
// to that call, so I wanted to see what the difference was before and after

console.log(ub.staticNetFilteringEngine.compile.mock.calls.map(args => args[0]));
expect(result).toBe('');

With the original regex, that printed out something like:

 [ 'ip6-localhost',
      'ip6-loopback',
      'ff00::0 ip6-localnet',
      'ff00::0 ip6-mcastprefix',
      'ff02::3 ip6-allhosts',
      '1493361689.rsc.cdn77.org',
      '30-day-change.com' ]

Okay now I see what was trying to be filtered out. So I picked a few that shouldn't be there, replaced the regex, and it passed. Well, in the middle there I got lazy and I didn't want to have 6 of the same expect(ub.staticNetFilteringEngine.compile).not.toHaveBeenCalledWith(arg, expect.any(Object)); calls. Also, I didn't really care what the writer argument was in this case, I just knew it was some kind of object. Thinking about it now, I would probably just mock out this.CompiledLineWriter() and then test that in utils.test.js. When I'm going back and adding to tests to existing code, I usually try to let as many of the functions do there thing if I don't care about the result while at the same time requiring as little of the code as possible to run whatever specific test I'm working on.

Now sure, I could've just studied the regex for a bit, but I'm not that smart and would rather see it in action.

I found writing tests for someone else's code is the best way for me to learn the codebase. This is what I did for a project called chromeless, which is sort of a "competitor" to Puppeteer.

I git cloned your repo, and first thing is, how do I launch tests?
Assuming you have Node installed (I use nvm):

# In the project root, this will install the dependences
npm install
# This will run the tests and generate a coverage report
npm test

And then I usually add a npm script to run jest in watch mode, which just re-runs the tests as files change.

npm run tdd

Let me know if you run into issues.

I saw that the build script needs rm -r $DES/js/tests, isn't possible to not copy tests in the first place by placing it outside $DES/js/?

Yes, it's just preference of directory structure. Another popular structure would be to put all the tests in a directory:

assets/
dist/
doc/
platform/
src/
tests/
  src/
    storage.test.js
  playform/
    chromium/
      vapi.test.js
tools/

Then you'd just need to change the testMatch property: https://github.com/geoffdutton/uBlock/blob/unit-tests-proposal/package.json#L36

Jest comes preconfigured to handle both structures out of the box. That line above in package.json is actually unnecessary. I've just run into fewer issues by explicitly stating the pattern that I want it to look for.

I have about 10 other files from src/ and a few from platform/ with a few tests that I can commit, I would just need to clean them up a bit. I just wanted to first see if this was of interest.

Also, here is one of the better articles I've read on testing philosophy/strategy: http://blog.codepipes.com/testing/software-testing-antipatterns.html

Edit: Sorry -- a bit long.

[jspenguin2017]

@geoffdutton There's nothing wrong with your unit tests, but mocking so many APIs will eventually result in this:

2 passed unit tests, but...

There need to be integration tests, running in a real browser on artificial (and maybe real) sites.

I use Puppeteer because it can load extensions and (used to) be able to auto-config them, I'm not sure if chromeless can do that.

The repo is here: https://github.com/NanoAdblocker/NanoTest
I definitely still need more tests, I stopped because CSP was broken in a Puppeteer update. It's fixed now though.

[geoffdutton]

@jspenguin2017 I see what you're saying now. Yes, you are right on the need to test the full product in a real browser in an automated way. Puppeteer would be great for that, but I think lower level unit tests can be complimentary.

For example, looking at this, I would need to do a lot of set up to make sure incrementing by 2 is what I really want.

But now say I wanted to test the correct tab was actually reloaded when the refresh button is clicked in your screen shot, that would be a disaster to test using something like jest. You'd have to mock out the entire chrome messaging system and such.

Another great use case for Puppeteer is "visual regression" sort of testing such as testing css media queries on various view port sizes.

I will take a look at your repo and see if I have any ideas.

[jspenguin2017]

@geoffdutton My worry is that uBO isn't really built on functional programming; one functionality can span across multiple files. Ideally you want many small pure functions and a few big controllers to tie them together. This way you can optimize smaller functions without needing to worry about breaking stuff.

But now it's too late to change, so we have to instead design tests around it. I checked your tests, I have to say they are not ideal. I don't have a better way but ideally you would run a function multiple times instead of require a file multiple time; that just look like a headache waiting to happen.

Also for code like this, it just feel really fragile and hacky:

chrome.runtime.getManifest.mockReturnValue({ version: '1.1.1.1' });

Ideally we don't want to mock browser APIs, instead, we should mock vAPI which we control. We can then use integration tests to make sure vAPI scripts are working correctly.

[geoffdutton]

one functionality can span across multiple files.

I guess I'd say that's why having unit tests in place to prove the "contract" of the functionality.

Also for code like this, it just feel really fragile and hacky:

Well the only thing we care about is that it's 4 digits, but I can see what you're saying.

Edit: to your point testing a file like this would be really fragile because there’s all the internal stuff going on that’s not exposed https://github.com/gorhill/uBlock/blob/master/src/js/assets.js

[uBlock-user]

I use right-Alt + m,

On Windows, Alt + 2+3+0 on numpad types µ

http://www.theasciicode.com.ar/

[jspenguin2017]

It's 181 for me. Could be font-dependent.

[uBlock-user]

Alt + 0181 also works.

[sebast889]

Reason for close? @gorhill not wanting unit tests or just that this particular issue is not needed anymore?