991442 - Add coverage threshold to test-agent

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::TestAgent, defect)

Description

[Kevin Grandon :kgrandon]

We would like to have an environment variable to have the reporter fail when coverage does not meet a certain threshold. E.g., 

make test-agent-test APP=myapp COVERAGE=1 COVERAGE_THRESHOLD=50

Comment 1

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

that's a cool feature! could we also support this feature on test-agent browser? then we don't need to run test-agent-server/test to set the threshold.

Comment 2

[Kevin Grandon :kgrandon]

Thanks for weighing in Yuren. I would like to see this run on Travis, so we will also need some kind of command-line support. If we can have this working on both the browser and command line, I think that would be great.

(It may also be nice to loop in the build tests to this as well, but I think we should get this working for a single app first)

Ideally we could have a json file somewhere that specifies an app -> minimum coverage threshold. E.g., 

{
  "calendar" : 50,
  "search" : 100
}

Comment 3

[Kevin Grandon :kgrandon]

Thanks Ricky!

Comment 4

[Ricky Chien [:rickychien] (inactive)]

Attachment: Gaia patch PR

Comment 5

[Ricky Chien [:rickychien] (inactive)]

Attachment: Js-test-agent PR

Comment 6

[Ricky Chien [:rickychien] (inactive)]

Attachment: coverage-passed.png

This bug is WIP.

Coverage thresholds result has been supported on test agent console.

Kevin, could you give me some advices for this? :)

Comment 7

[Ricky Chien [:rickychien] (inactive)]

Attachment: coveage-failed.png

Comment 8

[Ricky Chien [:rickychien] (inactive)]

I set a shared/test/unit/coveage-thresholds.json file to store min threshold as you mentioned on comment 2

Comment 9

[Kevin Grandon :kgrandon]

Wow, this is awesome. Not much to say besides I love it!

I wonder if we should put all apps with tests in the json file at like 1%, then letting app authors increase that value if they want?

Comment 10

[Ricky Chien [:rickychien] (inactive)]

Nice suggestion!

I've updated PR and set all apps in json file to 0%.

Kevin, would you like to review this patch or maybe it is more suitable to forward to Juilen?

Comment 11

[Kevin Grandon :kgrandon]

Comment on attachment 8428345 [details] [review]
Gaia patch PR

I think it would make more sense for Julien to review this patch. Thanks!

Comment 12

[Ricky Chien [:rickychien] (inactive)]

Attachment: Gaia-node-modules PR

Comment 13

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

since we have metadata.json on build system for each app, I suggest getting coverage thresholds from $APP/metadata.json if exists.

Comment 14

[Ricky Chien [:rickychien] (inactive)]

I'm fine with using metadata.json for coverage thresholds.

Kevin and Julien, I think I need your suggestions for that. :)

Comment 15

[Julien Wajsberg [:julienw]]

Sorry, my suggestions for what ?

Comment 16

[Ricky Chien [:rickychien] (inactive)]

We have two ways to set the coverage threshold for each app.

1. According to comment 2 by Kevin, having one setting file locate at shared/test/unit/coverage-thresholds.json to set all thresholds.

{
  browser: 60,
  search: 80,
  ...
}

2. As yurenju mentioned on comment 13, set up coverage-threshold in a metadata.json file for each app.

I'm fine with both two ways. So I want to hear any other suggestions!

Comment 17

[Julien Wajsberg [:julienw]]

I'm fine with both too, actually...

most other threshold files are in build/ (eg: build/jshint/xfail.list or build/csslint/xfail.list).

Comment 18

[Kevin Grandon :kgrandon]

Is metadata.json solely for the build system? I was under the impression that metadata.json was used for some mozApps functions? If there are no standardization issues, then metadata.json is fine by me.

Comment 19

[Ricky Chien [:rickychien] (inactive)]

Thanks! I decide to choose metadata.json as our setting files. PR will upload soon.

Comment 20

[Ricky Chien [:rickychien] (inactive)]

Attachment: Gaia PR

Comment 21

[Ricky Chien [:rickychien] (inactive)]

Attachment: Gaia-node-modules PR

Comment 22

[Ricky Chien [:rickychien] (inactive)]

Test this patch by copying a existed metadata.json in Gaia to some $APP folder then add a "coverage-threshold": 60 into metadata.json file.

Comment 23

[Julien Wajsberg [:julienw]]

Comment on attachment 8428346 [details] [review]
Js-test-agent PR

Hey Ricky,

so I was (at last !) looking at this. So first of all, this is clearly very cool !

But the threshold part doesn't work, maybe you forgot to add the correct file or to push the latest version of your branch? Anyway, you're using the wrong property in setupThresholds and as a result the threshold stays at 0.

I'm so sorry again for the delay and now that I looked at it once I should be able to look at the other requests way faster !

Comment 24

[Julien Wajsberg [:julienw]]

BTW I think we should have another filename than metadata.json. metadata.json is used by the build process so if it finds the file but does not have some expected properties, the profile build just fails.

Comment 25

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8435485 [details] [review]
Gaia PR

Hi Julien, 

I've updated my PR and add a metadata.json into bluetooth for testing. (I'll remove it before landing this patch)

I have no idea what's name is suitable for replacing the name of metadata.json. As Yuren mentioned comment 13, his suggestion is prefer to put threshold into metadata.json. I agreed with him. But it's fine if you have a better idea to rename it.

Comment 26

[Julien Wajsberg [:julienw]]

(In reply to Ricky Chien [:rickychien] from comment #25)
> Comment on attachment 8435485 [details] [review]
> Gaia PR
> 
> Hi Julien, 
> 
> I've updated my PR and add a metadata.json into bluetooth for testing. (I'll
> remove it before landing this patch)
> 
> I have no idea what's name is suitable for replacing the name of
> metadata.json. As Yuren mentioned comment 13, his suggestion is prefer to
> put threshold into metadata.json. I agreed with him. But it's fine if you
> have a better idea to rename it.

I'm fine with metadata.json if you change the build system to ignore errors if the build system does not find something (I think it's looking for the property "external"). But maybe it's better to have different json files for different purposes, so I'm fine with this too.

Comment 27

[Julien Wajsberg [:julienw]]

Comment on attachment 8435485 [details] [review]
Gaia PR

I'm fine with adding a coverage.json.

Then, to resolve the issue of resolving app names, we should add a step in the test-agent-config Makefile goal to add a mapping between app directories and app names. Either we generate an aggregated groups.json file in dev_apps/test-agent, or we can add it to the existing config.json. We could also add the test files paths inside these groups, for example...

We can do this in an external JS script of course.

The idea for this mapping is to group files in groups. Maybe we can even then later reuse this for running "APP=<app> make test-agent-test", or to display the test-agent UI.

What do you think of this idea?

Comment 28

[Ricky Chien [:rickychien] (inactive)]

See comment 13, Yuren preferred to reuse metadata.json if it exists. I think we will lost this benefit if we decide to create the coverage.json for each app. 

On the other hand, it's not a good idea to remove "external" property checking in Makefile. So maybe it's time to choose Kevin's suggestion on comment 2 that seems to be an easy way without doing mapping in Makefile.

Comment 29

[Ricky Chien [:rickychien] (inactive)]

And we can still get appName by assuming it is in the last word of path. Ex: if the path /Users/Ricky/Documents/gaia/apps/sms, we can make sure the last keyword "sms" is our module name.

Just put a coverage.json in somewhere and fill with

{
  "sms": 60,
  ...
}

and we need to figure out where could be a good place to put coverage.json.

Comment 30

[Julien Wajsberg [:julienw]]

(In reply to Ricky Chien [:rickychien] from comment #29)
> And we can still get appName by assuming it is in the last word of path. Ex:
> if the path /Users/Ricky/Documents/gaia/apps/sms, we can make sure the last
> keyword "sms" is our module name.
> 

That's what I thought at first; but then I thought 2 things:
* entry points: "communications/contacts" is what we use for launching tests for this app.
* we still need to have something gaia-agnostic in the test-agent

That's why I came up with this grouping idea.

Comment 31

[Ricky Chien [:rickychien] (inactive)]

In order to cover sub-modules such as communications/contacts is going to be more complicated in implementation. Our current covered modules is assumed to be the folder name at apps/<module> or dev_apps/<module>. This convention has been implemented in test-agent app, so whether test-agent web UI or test-agent console, we can notice coverage result has grouped by above convention.

So I think if this kind of scenario isn't urgently needed, we could choose above easy way to implement it. Otherwise, we have to consider modifying both test-agent web UI and console output. :)

I though that remove hard code like 'app/xxx/' is a gaia-agnostic solution; however, I found [1] (search "options.modulePattern") is a better solution which specifies a modulePattern regexp to extract a module name from the path. What do you think?


[1] https://github.com/ModelN/grunt-blanket-mocha

Comment 32

[Ricky Chien [:rickychien] (inactive)]

Another thing is that our coverage has been broken for some reason even though this old branch still can work properly. 

I prefer to continue on this old branch for this bug but fix broken issue in another bug.

Comment 33

[Julien Wajsberg [:julienw]]

(In reply to Ricky Chien [:rickychien] from comment #32)
> Another thing is that our coverage has been broken for some reason even
> though this old branch still can work properly. 
> 
> I prefer to continue on this old branch for this bug but fix broken issue in
> another bug.

agreed

(In reply to Ricky Chien [:rickychien] from comment #31)
> In order to cover sub-modules such as communications/contacts is going to be
> more complicated in implementation. Our current covered modules is assumed
> to be the folder name at apps/<module> or dev_apps/<module>. This convention
> has been implemented in test-agent app, so whether test-agent web UI or
> test-agent console, we can notice coverage result has grouped by above
> convention.
> 
> So I think if this kind of scenario isn't urgently needed, we could choose
> above easy way to implement it. Otherwise, we have to consider modifying
> both test-agent web UI and console output. :)


Is it that difficult to create a mapping file in the `test-agent-config:` goal and load it in test-agent ?

> 
> I though that remove hard code like 'app/xxx/' is a gaia-agnostic solution;
> however, I found [1] (search "options.modulePattern") is a better solution
> which specifies a modulePattern regexp to extract a module name from the
> path. What do you think?
> 
> 
> [1] https://github.com/ModelN/grunt-blanket-mocha

Problem with this is that this will run the tests in node and not in the test-agent... which will give issues :/

Comment 34

[Ricky Chien [:rickychien] (inactive)]

(In reply to Julien Wajsberg [:julienw] (PTO until 7th October, ask :steveck or :azasypkin for SMS stuff) from comment #33)
> (In reply to Ricky Chien [:rickychien] from comment #32)
> 
> Is it that difficult to create a mapping file in the `test-agent-config:`
> goal and load it in test-agent ?

No, but I think we can make use of the "options.modulePattern" strategy in grunt-blanket-mocha so that we could extract appName from regexp instead of generating a mapping file in the `test-agent-config:`.

> 
> Problem with this is that this will run the tests in node and not in the
> test-agent... which will give issues :/

Oh, I meant that we could implement this "options.modulePattern" feature in our test-agent to get rid of generating a mapping file rather than importing grunt-blanket-mocha. :)

Comment 35

[Julien Wajsberg [:julienw]]

If this can work fine without too much code, then I'm ok with this :)

Comment 36

[Ricky Chien [:rickychien] (inactive)]

Attachment: Gaia PR

PR is coming!

Comment 37

[Ricky Chien [:rickychien] (inactive)]

After this job is done, we are going to switch to bug 1021567 enabling coverage watcher. If someone is writing their code while launching test-agent server, it could output live coverage report on console.

Comment 38

[Ricky Chien [:rickychien] (inactive)]

If this patch is ok to be landed, can we land it before solving bug 1081065?

Because I thank that patch won't impact our test-agent or coverage behaviors. :)

Comment 39

[Julien Wajsberg [:julienw]]

Sorry that I didn't have time to look at your patch for some days; I was away last thursday and friday and today I had to catch up.

I'll look at your patch very soon (I don't say "tomorrow" because it can be wednesday too).

Sorry again :/

Comment 40

[Julien Wajsberg [:julienw]]

The code looks right but it's difficult to test without bug 1081065. Therefore I'd rather fix bug 1081065 before.

Comment 41

[Ricky Chien [:rickychien] (inactive)]

PR has been rebase to master and it works properly in my mac! 

After resolving bug 1081065, we can set about landing this bug. :)

Comment 42

[Julien Wajsberg [:julienw]]

(yeah Ricky, it's still in my radar... but rather low priority :/)

Comment 43

[Julien Wajsberg [:julienw]]

Hey Ricky,

I finally took the time to look at it. It looks good as is but...

I found there is a big flaw: one threshold is checked per file... For the SMS application, for example, one file (startup.js) has a score of 16%, but most other files are above 90% (with some around 70 and others around 80).

Worst: some files that are in /shared are checked too. However, we don't really test them, it just happens that we run them as part of some tests. And since we don't really test them they have a very low coverage. For the SMS application, lazy_loader.js is one such file (it has 8%).

I think the only sane solution is to have a per-file configuation file, like we do for other linters. And have a way to automatically generate it (like [1]).

Since this sounds to be some work, maybe we should land your patch, but checking the average score instead of individual files. But we won't be able to do bug 1021567 until we have the additional work done.

Tell me what you think!

[1] https://github.com/mozilla-b2g/gaia/blob/master/build/jshint/README.md#how-to-generate-the-xfaillist-from-scratch

Comment 44

[Ricky Chien [:rickychien] (inactive)]

I prefer to set it passed by all file should pass the our module-threshold score rather than average score.
I think that makes sense and library such as grunt-blanket-mocha [1] which has same structure.

On the other hand, I agree with you to skip /shrared resources. Actually, we can skip them by setting up blanket flags without adding such complicated configuration.

[1] https://github.com/ModelN/grunt-blanket-mocha

Comment 45

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8502964 [details] [review]
Gaia PR

Filter for skipping /shared/... resources has been added.

Comment 46

[Julien Wajsberg [:julienw]]

Comment on attachment 8502964 [details] [review]
Gaia PR

yep, let's do this

it's not perfect (for example, we can't get the coverage for shared files that are tested in apps/sharedtest), but it's already something and we can build on it.

Thanks for your patience!

Note: I haven't tried the code with your last changes so please test thoroughly before landing.

Comment 47

[Julien Wajsberg [:julienw]]

Don't forget the last comment in the patch for test-agent too :)

Comment 48

[Ricky Chien [:rickychien] (inactive)]

Thanks for your patient too. Your last comment in test-agent has fixed.

Comment 49

[Ricky Chien [:rickychien] (inactive)]

Merged gaia-node-modules.

https://github.com/mozilla-b2g/gaia-node-modules/commit/81cc32456dd13f2cd099ad092b442eed5b17287d

Merged js-test-agent.

https://github.com/mozilla-b2g/js-test-agent/commit/85a20aadefef6c570b180df9611ad82cfb7d82e8

Merged gaia.

https://github.com/mozilla-b2g/gaia/commit/f8b2a9d3d261f8914b16bceb1e75623a096ed306

Comment 50

[Ricky Chien [:rickychien] (inactive)]

Attachment: Gaia-node-modules-revision

Follow-up with updating gaia-node-modules.revision

Comment 51

[Ricky Chien [:rickychien] (inactive)]

Merged gaia-node-modules.revision.

https://github.com/mozilla-b2g/gaia/commit/009cf66612d219985338a9e9f9f5d9c2b9fc0d3f