1110636 - ./mach test gtest && ./mach gtest test-pathes is not work || ./mach test test-pathes will not invoke gtest

Status: NEW

(Testing :: GTest, defect)

Description

[u459114]

1. "./mach test" command is not work for gtest
> cjcool@~/repository/gecko-dev (git/master u= origin/master)$ ./mach test gfx/test/gtest
> UNKNOWN TEST: gfx/tests/gtest
> I was unable to find tests in the argument(s) given.
(Looks like gtest-cases are not put in all-tests.json)

2. ./mach gtest path-to-gtest-folder
> cjcool@~/repository/gecko-dev (git/master u= origin/master)$ ./mach gtest gfx/tests/gtest
> ...
> Running GTest tests...
> Note: Google Test filter = gfx/tests/gtest
> [==========] Running 0 tests from 0 test cases.
> [==========] 0 tests from 0 test cases ran. (1 ms total)
> [  PASSED  ] 0 tests.
> Finished running GTest tests.
There is no test cases been executed.

It makes gtests unique among testing suites. I have to explicit choose test case name pattern as "mach test" command argument.

As a user, when I submit the following command -
  $ ./mach test gfx
I expect running through all testing suites, includes reftest/ mochitest and gtest, under gfx folder.

We might need to generate a map of gtest in build process for this purpose. Any suggestion?

PS: all-tests.json is really huge... The size of this file is around 45MB, while only one-line in this file. I can't open this file and do a search in any editors.

Comment 1

[(not currently active) Ted Mielczarek]

This is tricky for a number of reasons right now. For one, the only thing moz.build knows about gtest sources is that there are source files with FINAL_LIBRARY = 'xul-gtest':
http://dxr.mozilla.org/mozilla-central/source/dom/media/gtest/moz.build#41

We could swap this out for something that's more visible to moz.build, that wouldn't be too bad.

The second part that's tricky is that gtest filtering uses test names (as defined in source files), which have nothing to do with the path to the source files in the tree:
http://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/mach_commands.py#660

For example, to run only this media test:
http://dxr.mozilla.org/mozilla-central/source/dom/media/gtest/TestAudioCompactor.cpp#84

You'd specify "Media::AudioCompactor_4000". I don't know how we could possibly make mach aware of this short of instrumenting the build in some way.

Comment 2

[u459114]

FINAL_LIBRARY = 'xul-gtest' gives us a hint there are test cases in this context(moz.build), but we still don't know the name of test cases in Context.SOURCES.

One idea is adding one more property, e.g. Context.GTESTCASENAME, in Context.
In moz.build, developers need to manually write down test case name in this folder.
Here is an example, in dom/media/gtest/moz.build, insert the following line
  GTESTCASENAME = (Media, GeckoMediaPlugins, MP4Demuxer, MP4Reader, VideoSegment, WebMBuffered) 


Create a map between CONTEXT.SOURCES and CONTEXT.GTESTCASENAME and store this map in a storage file.

Comment 3

[Mike Hommey [:glandium]]

You can be sure this will be badly maintained and will often not match reality. I'd rather have something automatic, like reading the source files and looking for TEST(testname, subtestname) patterns. Gtest has plenty of macros that you essentially have to use if you want your tests to work, and all tests should be declared using them, so they should be easy to find.

Comment 4

[(not currently active) Ted Mielczarek]

I was wondering if we could coerce gtest to list all the tests it knows about and their corresponding source files, but it doesn't appear to even store the source file information, so that's unlikely to work.

Comment 5

[Benoit Girard (:BenWa)]

We could look at the symbol table for xul-gtest and run a regex on it. If it doesn't we can make sure that the function name emmited by TEST(a,b) patch a simple regex, but they might already. Something like nm xul-gtest | grep ^gtest_(.*)_(.*)

Better yet we could do it on the object files themselves and not require linking gtest.

Comment 6

[u459114]

Thanks for all your suggestions

Here is a list of options of make path-filter mapping work
1. (comment 2) implicit add test case name in moz.build.
2. (comment 3) gtest source code parsing(by clang?) 
3. (comment 5) gtest object file parsing(nm/ objdump)
4. Rewrite TEST marco, adding source path attrribute(via __FILE__) into TestInfo.

Comment 7

[u459114]

(In reply to C.J. Ku[:cjku] from comment #6)
> Here is a list of options of make path-filter mapping work
> 1. (comment 2) implicit add test case name in moz.build.
typo: explicit

Comment 8

[Mike Hommey [:glandium]]

(In reply to C.J. Ku[:cjku] from comment #6)
> 2. (comment 3) gtest source code parsing(by clang?) 

Grep or similar "parsing" from python is likely enough

Comment 9

[u459114]

Attachment: (WIP) persist sourch-path-to-gtest-filter map

WIP1: search gtest source files and store source-path-to-filter mapping into gtestmap file

Comment 10

[u459114]

Attachment: (WIP) persist sourch-path-to-gtest-filter map

Comment 11

[u459114]

1. Integrate gtestmap file with make system
2. @Command('gtest'/'test'...): translate path into gtest-filter, and run gtest cases under the specific folder.

Comment 12

[u459114]

Attachment: (WIP2) persist sourch-path-to-gtest-filter map

Comment 13

[u459114]

Attachment: (WIP3) persist sourch-path-to-gtest-filter map

Comment 14

[u459114]

TBD
1. test case
2. @Command('test'

Comment 15

[u459114]

If anyone wants to give it a try, here are steps:

1. Apply attachment 8543536 [details] [diff] [review]
2. ./mach build - generate gtestmap file under obj-folger.
3. ./mach gtest gfx - run all test cases in gfx folder
   Or "./mach gtest dom/media" which run through all media recorder gtest cases.

Comment 16

[u459114]

Attachment: (WIP3) persist sourch-path-to-gtest-filter map

Comment 17

[u459114]

Comment on attachment 8543758 [details] [diff] [review]
(WIP3) persist sourch-path-to-gtest-filter map

Hi Mike, would you mind have a look on this patch?

Comment 18

[u459114]

Attachment: (WIP3) persist sourch-path-to-gtest-filter map

autopep8 gtest_map.py

Comment 19

[u459114]

Attachment: (WIP4) Covert a path to gtest filters

Hi, here is a version of grepping gtest source code and create a map file to store relation between source code path and testcase name.
May I have your opinion here? thanks

Comment 20

[Benoit Girard (:BenWa)]

Comment on attachment 8544446 [details] [diff] [review]
(WIP4) Covert a path to gtest filters

Review of attachment 8544446 [details] [diff] [review]:
-----------------------------------------------------------------

::: Makefile.in
@@ +91,4 @@
>  	@$(TOUCH) $@
>  
>  include backend.RecursiveMakeBackend.pp
> +include gtestdeps.mk

gtestdeps is missing from the patch.

Comment 21

[u459114]

(In reply to Benoit Girard (:BenWa) from comment #20)
> Comment on attachment 8544446 [details] [diff] [review]
> (WIP4) Covert a path to gtest filters
> 
> Review of attachment 8544446 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: Makefile.in
> @@ +91,4 @@
> >  	@$(TOUCH) $@
> >  
> >  include backend.RecursiveMakeBackend.pp
> > +include gtestdeps.mk
> 
> gtestdeps is missing from the patch.

Just like backend.RecursiveMakeBackend.pp, it's a auto-generated file. gtest_mapper.save produce this file after parsing all Contexts

Comment 22

[Benoit Girard (:BenWa)]

Comment on attachment 8544446 [details] [diff] [review]
(WIP4) Covert a path to gtest filters

Review of attachment 8544446 [details] [diff] [review]:
-----------------------------------------------------------------

Are we scanning each file in our tree here? Adding an extra pass on all our sources is not ideal.

::: python/mozbuild/mozbuild/frontend/gtest_mapper.py
@@ +8,5 @@
> +from .context import Context
> +from ..makeutil import Makefile
> +
> +MAP_FILENAME = 'gtestmap'
> +DEP_FILENAME = 'gtestdeps.mk'

If this is a generated dep file you should use a more standard extension IMO. Something like .pp. It's a bit confusing because with .mk I expect to find a Makefile in the tree but with .pp I know what we're generating.

You're only generating dependencies in that file right?

::: python/mozbuild/mozbuild/mach_commands.py
@@ +698,5 @@
>              os.makedirs(cwd)
>  
> +        # Convert gtest_filter from path to filter.
> +        import mozbuild.frontend.gtest_mapper as gm
> +        gtest_filter = gm.load(self.topsrcdir, self.topobjdir, gtest_filter)

NIT: It's usually better to not use the same variable for something that has too different format. I'd recommend having a different name for gtest_path.

Comment 23

[u459114]

(In reply to Benoit Girard (:BenWa) from comment #22)
> Comment on attachment 8544446 [details] [diff] [review]
> (WIP4) Covert a path to gtest filters
> 
> Review of attachment 8544446 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Are we scanning each file in our tree here? Adding an extra pass on all our
> sources is not ideal.
Nope, only scan source files whose target lib is xul-gtest.

if not isinstance(context, Context) or \
   'xul-gtest' != context.get('FINAL_LIBRARY'):
   continue << skip file parsing if target lib is not xul-gtest

Comment 24

[u459114]

Comment on attachment 8544446 [details] [diff] [review]
(WIP4) Covert a path to gtest filters

Review of attachment 8544446 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/frontend/gtest_mapper.py
@@ +9,5 @@
> +from ..makeutil import Makefile
> +
> +MAP_FILENAME = 'gtestmap'
> +DEP_FILENAME = 'gtestdeps.mk'
> +

Yes, only dependencies.

Will rename this file according

::: python/mozbuild/mozbuild/mach_commands.py
@@ +699,5 @@
>  
> +        # Convert gtest_filter from path to filter.
> +        import mozbuild.frontend.gtest_mapper as gm
> +        gtest_filter = gm.load(self.topsrcdir, self.topobjdir, gtest_filter)
> +

Understand. Will do

Comment 25

[Benoit Girard (:BenWa)]

(In reply to C.J. Ku[:cjku] from comment #23)
> (In reply to Benoit Girard (:BenWa) from comment #22)
> > Comment on attachment 8544446 [details] [diff] [review]
> > (WIP4) Covert a path to gtest filters
> > 
> > Review of attachment 8544446 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Are we scanning each file in our tree here? Adding an extra pass on all our
> > sources is not ideal.
> Nope, only scan source files whose target lib is xul-gtest.
> 
> if not isinstance(context, Context) or \
>    'xul-gtest' != context.get('FINAL_LIBRARY'):
>    continue << skip file parsing if target lib is not xul-gtest

Ohh that's clever! I like it.

Comment 26

[u459114]

Attachment: (WIP5) Covert path to gtest filters

Comment 27

[u459114]

Attachment: Part 1. Covert path-paramenter into gtest_filter

Hi Mike/ Gps,
Please help tp review this patch when you have time.

By this change, user can
1.
$ ./mach gtest gfx/test # a test path is given
To run throught all gtest cases in gfx/test folder
2.
./mach gtest -f APZC* # a test filter is given
To run all test cases matched APZC*
3. 
./mach test gfx/test
To run all tests, include gtest, in gfx/test
4.
./mach test gtest
which is equal to './mach gtest -f *' or './mach gtest .'

Comment 28

[Gregory Szorc [:gps]]

Comment on attachment 8547410 [details] [diff] [review]
Part 1. Covert path-paramenter into gtest_filter

I'm busy this week. glandium can look at this.