Closed Bug 991442 Opened 6 years ago Closed 5 years ago

Add coverage threshold to test-agent

Categories

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

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: kgrandon, Assigned: rickychien)

References

Details

Attachments

(6 files, 3 obsolete files)

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
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.
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
}
Assignee: nobody → ricky060709
Status: NEW → ASSIGNED
Thanks Ricky!
Attached file Gaia patch PR (obsolete) —
Attached image 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? :)
Flags: needinfo?(kgrandon)
I set a shared/test/unit/coveage-thresholds.json file to store min threshold as you mentioned on comment 2
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?
Flags: needinfo?(kgrandon)
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?
Flags: needinfo?(kgrandon)
Comment on attachment 8428345 [details] [review]
Gaia patch PR

I think it would make more sense for Julien to review this patch. Thanks!
Attachment #8428345 - Flags: review?(felash)
Flags: needinfo?(kgrandon)
Attachment #8428346 - Flags: review?(felash)
Summary: Add coverage threshold to test-agent-test make target → Add coverage threshold to test-agent
Attached file Gaia-node-modules PR (obsolete) —
since we have metadata.json on build system for each app, I suggest getting coverage thresholds from $APP/metadata.json if exists.
I'm fine with using metadata.json for coverage thresholds.

Kevin and Julien, I think I need your suggestions for that. :)
Flags: needinfo?(kgrandon)
Flags: needinfo?(felash)
Sorry, my suggestions for what ?
Flags: needinfo?(felash)
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!
I'm fine with both too, actually...

most other threshold files are in build/ (eg: build/jshint/xfail.list or build/csslint/xfail.list).
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.
Flags: needinfo?(kgrandon)
Thanks! I decide to choose metadata.json as our setting files. PR will upload soon.
Attached file Gaia PR (obsolete) —
Attachment #8428345 - Attachment is obsolete: true
Attachment #8428345 - Flags: review?(felash)
Attached file Gaia-node-modules PR
Attachment #8434656 - Attachment is obsolete: true
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.
Depends on: 1021567
Blocks: 1021567
No longer depends on: 1021567
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 !
Attachment #8428346 - Flags: review?(felash)
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 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.
Attachment #8435485 - Flags: review?(felash)
(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 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?
Attachment #8435485 - Flags: review?(felash)
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.
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.
Flags: needinfo?(felash)
(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.
Flags: needinfo?(felash)
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
Flags: needinfo?(felash)
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.
(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 :/
Flags: needinfo?(felash)
(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. :)
Flags: needinfo?(felash)
If this can work fine without too much code, then I'm ok with this :)
Flags: needinfo?(felash)
Attached file Gaia PR
PR is coming!
Attachment #8435485 - Attachment is obsolete: true
Attachment #8502964 - Flags: review?(felash)
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.
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. :)
Flags: needinfo?(felash)
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 :/
Flags: needinfo?(felash)
The code looks right but it's difficult to test without bug 1081065. Therefore I'd rather fix bug 1081065 before.
Depends on: 1081065
PR has been rebase to master and it works properly in my mac! 

After resolving bug 1081065, we can set about landing this bug. :)
Flags: needinfo?(felash)
Flags: needinfo?(felash)
(yeah Ricky, it's still in my radar... but rather low priority :/)
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
Attachment #8502964 - Flags: review?(felash)
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 on attachment 8502964 [details] [review]
Gaia PR

Filter for skipping /shared/... resources has been added.
Attachment #8502964 - Flags: review?(felash)
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.
Attachment #8502964 - Flags: review?(felash) → review+
Don't forget the last comment in the patch for test-agent too :)
Thanks for your patient too. Your last comment in test-agent has fixed.
Follow-up with updating gaia-node-modules.revision
Merged gaia-node-modules.revision.

https://github.com/mozilla-b2g/gaia/commit/009cf66612d219985338a9e9f9f5d9c2b9fc0d3f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.