1242565 - refactor gtest bash scripts and main files

Status: RESOLVED FIXED

(NSS :: Test, defect)

Description

[Franziskus Kiefer [:franziskus]]

All gtests use a more or less identical *_gtest.sh bash script to run the tests. This should be refactored and cleaned up.

Comment 1

[Franziskus Kiefer [:franziskus]]

*_gtest.cc files are more or less identical as well

Comment 2

[Franziskus Kiefer [:franziskus]]

Attachment: gtests-bash-reorga.patch

first draft of a unified gtest bash script to remove redundant code. Jed can you give first feedback on this? The next step is then to unify the main gtest files to remove those redundant main files.

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8737236 [details] [diff] [review]
gtests-bash-reorga.patch

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

A few minor comments, but overall I like this.  It will be nice to have a lot less copying-and-pasting to do for the next new gtest.

::: tests/all.sh
@@ +159,5 @@
>      NSS_ENABLE_PKIX_VERIFY="1"
>      export NSS_ENABLE_PKIX_VERIFY
>  
>      TESTS="${ALL_TESTS}"
> +    TESTS_SKIP="cipher dbtests sdr crmf smime merge multinit gtests"

Are we sure libpkix-ness doesn't matter for any of the other gtests?

::: tests/gtests/gtests.sh
@@ +72,5 @@
> +
> +############################## gtest_init ##############################
> +# local shell function to initialize this script
> +# $1 is the component(s) to initialise
> +# $2 is the folder

$2 is the tests_results subdirectory for this test run (cycle + test), I think?

Also, it would be nice to use `local` to give these variables names.  These are all bash scripts (judging by the #!) so that's not a compatibility problem.

@@ +82,5 @@
> +  if [ -z "${CLEANUP}" ] ; then     # if nobody else is responsible for
> +      CLEANUP="${SCRIPTNAME}"       # cleaning this script will do it
> +  fi
> +
> +  html_head libnss"${1}" Gtests

Nit: this title might not always make much sense (libnssder? libnssssl?)

@@ +84,5 @@
> +  fi
> +
> +  html_head libnss"${1}" Gtests
> +
> +  if [[ ! -d "${2}" ]] && [[ "${1}" != ssl ]]; then

This is [[ when all the other tests are [ and I don't understand why.  Also, && works, but [ can do both tests in one expression with -a, not that it makes much difference.

@@ +134,5 @@
> +fi
> +
> +# util
> +gtest_init "util" "${UTILGTESTDIR}"
> +gtest_start "util" "${UTILGTESTDIR}"

It would be nice to do something about these *GTESTDIR variables; right now each gtest needs boilerplate in tests/common/init.sh.  But that could be a followup.

Comment 4

[Franziskus Kiefer [:franziskus]]

Attachment: gtests-organisation.patch

this is a re-orga patch for gtests so that we don't have to copy code all the time. Flagging everyone who's writing gtests for review so they can say something to it (no full review required from everybody).
Main points:
i) a single gtests.cc that runs all tests
ii) a single gtests.sh to kick off gtests
iii) running a subset of tests is done setting NSS_GTESTS to the value for --gtest_filter [1]
iv) to make iii work the next patch renames tests that allow for easier filtering


[1] https://github.com/google/googletest/blob/d225acc90bc3a8c420a9bcd1f033033c1ccd7fe0/googletest/docs/V1_7_AdvancedGuide.md#running-a-subset-of-the-tests

Comment 5

[Franziskus Kiefer [:franziskus]]

Attachment: gtests-renaming-tests.patch

This patch renames gtests for easier filtering (see previous comment), i.e.:
* TLS_* for ssl_gtests
* Util_* for util_gtests
* DER_* for der_gtests
* PK11_* for pk11_gtests

Comment 6

[Franziskus Kiefer [:franziskus]]

Patches are also here:
* re-orga https://codereview.appspot.com/294340043
* renaming https://codereview.appspot.com/297100043

Comment 7

[Martin Thomson [:mt:]]

Comment on attachment 8745896 [details] [diff] [review]
gtests-organisation.patch

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

I think that I would prefer this with a single binary for each directory.  It's not about running tests.  The filter is enough for running a subset of tests.  The setup for ssl is a burden for other tests.  Most directories don't need the DB.

A common script is good, that can run all the different binaries.  That only needs to be a simple list and for loop.

A common main function is also good, but it would need to be slightly more flexible (if -d is specified, run with the DB, otherwise run without).

I don't know why you think it necessary to use env vars for filtering.  The scripts (and binaries) can take arguments if it comes to that.  The simplest possible argument-processing logic would be --gtest-filter=$(for i in "$@"; do echo -n ":*$i*"; done | cut -c 2- -).  Then you can specify individual test names, or part thereof on the command line and run them.

For me, when debugging, I run the binary directly and would hope to still do the same.

::: external_tests/gtests/gtests.cc
@@ +17,5 @@
> + * NSS_GTESTS acts as filter (--gtest_filter):
> + *            e.g. NSS_GTESTS="PK11_*:DER_*" runs all PK11 and DER gtests
> + */
> +int main(int argc, char **argv) {
> +  /* NSS_GTESTS contains the regex filter "--gtest_filter=NSS_GTESTS" */

You should pull this in from argv instead.

@@ +40,5 @@
> +    g_working_dir_path = workdir;
> +
> +  for (int i = 0; i < argc; i++) {
> +    if (!strcmp(argv[i], "-d")) {
> +        printf("dir: %s\n", argv[i + 1]);

potential array overflow error here

@@ +46,5 @@
> +      ++i;
> +    }
> +  }
> +
> +  NSS_Initialize(g_working_dir_path.c_str(), "", "", SECMOD_DB, NSS_INIT_READONLY);

Most of the tests don't need a DB, so you are burdening them with one.

::: external_tests/gtests/manifest.mn
@@ +13,5 @@
> +
> +CPPSRCS = \
> +	gtests.cc \
> +	$(wildcard */*.cpp) \
> +	$(wildcard */*.cc) \

This isn't good.  These files avoid having functions or macros.  We need to list files explicitly.

Comment 8

[Franziskus Kiefer [:franziskus]]

(In reply to Martin Thomson [:mt:] from comment #7)
> Comment on attachment 8745896 [details] [diff] [review]
> The filter is enough for running a subset of tests.  The setup for ssl is 
> a burden for other tests.  Most directories don't need the DB.
right, we can fix that by not creating the db if not needed.

> I think that I would prefer this with a single binary for each directory. 
> A common script is good, that can run all the different binaries.  That only
> needs to be a simple list and for loop.
If we want this, we still have to copy all the code for the main file and I don't really see a reason for that.

> A common main function is also good, but it would need to be slightly more
> flexible (if -d is specified, run with the DB, otherwise run without).
Yes we can do that (also in the bash script already).

> I don't know why you think it necessary to use env vars for filtering.  The
> scripts (and binaries) can take arguments if it comes to that.  The simplest
> possible argument-processing logic would be --gtest-filter=$(for i in "$@";
> do echo -n ":*$i*"; done | cut -c 2- -).  Then you can specify individual
> test names, or part thereof on the command line and run them.
Well, you can do the same now with NSS_GTESTS=TLS_* etc. Main reason for the env var was convenience as it doesn't require to change the bash scripts.

> For me, when debugging, I run the binary directly and would hope to still do
> the same.
You can still do that by running NSS_GTESTS=TLS_* gtests -d [dir]

Comment 9

[Martin Thomson [:mt:]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #8)
> > I think that I would prefer this with a single binary for each directory. 
> > A common script is good, that can run all the different binaries.  That only
> > needs to be a simple list and for loop.
> If we want this, we still have to copy all the code for the main file and I
> don't really see a reason for that.

You don't have to create a copy, just link against the .o multiple times.  (Include ../common/gtest_main.cc from every subdirectory.)
 
> > I don't know why you think it necessary to use env vars for filtering.  The
> > scripts (and binaries) can take arguments if it comes to that.  The simplest
> > possible argument-processing logic would be --gtest-filter=$(for i in "$@";
> > do echo -n ":*$i*"; done | cut -c 2- -).  Then you can specify individual
> > test names, or part thereof on the command line and run them.
> Well, you can do the same now with NSS_GTESTS=TLS_* etc. Main reason for the
> env var was convenience as it doesn't require to change the bash scripts.

That's not a great reason.  The env var doesn't make it easier for us to run the scripts.  And that's what is important when it comes to filtering.
 
> > For me, when debugging, I run the binary directly and would hope to still do
> > the same.
> You can still do that by running NSS_GTESTS=TLS_* gtests -d [dir]

That doesn't let me run using a debugger though, does it?  Or valgrind, or rr.

Comment 10

[Eric Rescorla (:ekr)]

Comment on attachment 8745898 [details] [diff] [review]
gtests-renaming-tests.patch

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

Can you put this up on Rietveld.

Comment 11

[Martin Thomson [:mt:]]

Comment on attachment 8745898 [details] [diff] [review]
gtests-renaming-tests.patch

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

I'm ambivalent on this.

Comment 12

[Franziskus Kiefer [:franziskus]]

(In reply to Eric Rescorla (:ekr) from comment #10)
> Comment on attachment 8745898 [details] [diff] [review]
> gtests-renaming-tests.patch
> 
> Review of attachment 8745898 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you put this up on Rietveld.

see comment 6

Comment 13

[Franziskus Kiefer [:franziskus]]

(In reply to Martin Thomson [:mt:] from comment #9)
> (In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #8)
> > > I think that I would prefer this with a single binary for each directory. 
> > > A common script is good, that can run all the different binaries.  That only
> > > needs to be a simple list and for loop.
> > If we want this, we still have to copy all the code for the main file and I
> > don't really see a reason for that.
> 
> You don't have to create a copy, just link against the .o multiple times. 
> (Include ../common/gtest_main.cc from every subdirectory.)
yeah, that should work. requires some (imho unnecessary) work when creating new tests though.

> > Well, you can do the same now with NSS_GTESTS=TLS_* etc. Main reason for the
> > env var was convenience as it doesn't require to change the bash scripts.
> 
> That's not a great reason.  The env var doesn't make it easier for us to run
> the scripts.  And that's what is important when it comes to filtering.
I'm not sure that I see why it complicates running scripts. We currently always run all gtests or none anyway, so no need to filter here anyway (only the ssl gtests need considerable time). And if we want to enable/disable certain tests we can export the env var in the script as filter. But I'm not too fuzzy about this, a command line flag works as well

> > You can still do that by running NSS_GTESTS=TLS_* gtests -d [dir]
> 
> That doesn't let me run using a debugger though, does it?  Or valgrind, or
> rr.
I don't see a reason why not. Running e.g. gdb --args gtests -d [dir] (setting the env var with set environment NSS_GTESTS=TLS_*) works just fine for me.

Comment 14

[Franziskus Kiefer [:franziskus]]

Attachment: gtests-organisation.patch

so how about this? I left ssl_gtests as is because they're very different from all other gtests and merged the others in a way that still produces separate binaries but doesn't require a main for each gtest.
This way we only have to add Makefile/manifest.mn/tests for each new gtest component.

Comment 15

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8745898 [details] [diff] [review]
gtests-renaming-tests.patch

this is not needed anymore

Comment 16

[Martin Thomson [:mt:]]

Comment on attachment 8747697 [details] [diff] [review]
gtests-organisation.patch

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

::: external_tests/der_gtest/manifest.mn
@@ +7,5 @@
>  MODULE = nss
>  
>  CPPSRCS = \
>        der_getint_unittest.cc \
> +      ../gtests.cc \

../common/gtests.cc perhaps?

::: external_tests/manifest.mn
@@ +11,3 @@
>  	util_gtest \
> +	pk11_gtest \
> +	ssl_gtest \

Is this a whitespace change that I can't see?

::: tests/gtests/gtests.sh
@@ +39,5 @@
> +# Local function to actually start the test
> +####################################################################
> +gtest_start()
> +{
> +  for i in $(find ${BINDIR} -name '*_gtest' -printf "%f\n"); do

An explicit list would be best here.  And this won't work on windows due to .exe extension (it will with a list).

Comment 17

[Franziskus Kiefer [:franziskus]]

> Is this a whitespace change that I can't see?
Yes, this is a whitespace change only (was mixed tabs and spaces).

landed as https://hg.mozilla.org/projects/nss/rev/830d52622b91

Comment 18

[Franziskus Kiefer [:franziskus]]

Attachment: gtest-followup.patch

In this follow up patch I build the gtests main file separately and add it in a new EXTRA_OBJS to the build. This fixes the additional build folder problem.
What do you think?

Comment 19

[Martin Thomson [:mt:]]

Comment on attachment 8753493 [details] [diff] [review]
gtest-followup.patch

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

WFM

::: external_tests/common/manifest.mn
@@ +16,5 @@
> +REQUIRES = gtest
> +
> +EXTRA_LIBS = $(DIST)/lib/$(LIB_PREFIX)gtest.$(LIB_SUFFIX)
> +
> +# NOTE: this is not actually used but required to build gtests.o

A library would be better here, I think.  Sure, we won't use it, but it won't be doing a final link, which should be cheaper.

::: external_tests/pk11_gtest/manifest.mn
@@ +20,5 @@
>  
>  PROGRAM = pk11_gtest
>  EXTRA_LIBS = $(DIST)/lib/$(LIB_PREFIX)gtest.$(LIB_SUFFIX)
> +
> +EXTRA_OBJS = ../common/$(OBJDIR)/gtests$(OBJ_SUFFIX)

Can you define EXTRA_OBJS in gtests.mk instead?

Comment 20

[Martin Thomson [:mt:]]

Comment on attachment 8753493 [details] [diff] [review]
gtest-followup.patch

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

One final suggestion.

::: coreconf/rules.mk
@@ -240,5 @@
>  
>  $(PROGRAM): $(OBJS) $(EXTRA_LIBS)
>  	@$(MAKE_OBJDIR)
>  ifeq (,$(filter-out _WIN%,$(NS_USE_GCC)_$(OS_TARGET)))
> -	$(MKPROG) $(subst /,\\,$(OBJS)) -Fe$@ -link $(LDFLAGS) $(XLDFLAGS) $(subst /,\\,$(EXTRA_LIBS) $(EXTRA_SHARED_LIBS) $(OS_LIBS))

Rather than change this here, how about adding (to gtests.mk again):

OBJS += ../common/$(OBJDIR)/gtests$(suffix)

Comment 21

[Franziskus Kiefer [:franziskus]]

landed the follow-up as https://hg.mozilla.org/projects/nss/rev/27342403fb54