Closed Bug 1057584 Opened 7 years ago Closed 7 years ago

Add gtest framework and initial tests optionally compiled

Categories

(NSS :: Test, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: cviecco, Assigned: cviecco)

Details

Attachments

(4 files, 14 obsolete files)

2.60 MB, patch
cviecco
: review+
Details | Diff | Splinter Review
9.06 KB, patch
Details | Diff | Splinter Review
7.81 KB, patch
cviecco
: review+
Details | Diff | Splinter Review
40.67 KB, patch
cviecco
: review+
Details | Diff | Splinter Review
Eric Rescorla wrote an initial set of tests using the gtest framework. This will allow us to expand test coverage to where the tls connection is not working as expected (broken tls/dtls)
Assignee: nobody → cviecco
Attached patch add-gtest-framework (obsolete) — Splinter Review
Attached patch add-gtest-db (obsolete) — Splinter Review
Attached patch build-system-patches (obsolete) — Splinter Review
Attached patch test-code (obsolete) — Splinter Review
Attached patch build-system-patches (v2) (obsolete) — Splinter Review
Attachment #8477694 - Attachment is obsolete: true
Comment on attachment 8477687 [details] [diff] [review]
add-gtest-framework

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

This is a bulk import of gtest (from svn).
Attachment #8477687 - Flags: review?(ryan.sleevi)
Comment on attachment 8477688 [details] [diff] [review]
add-gtest-db

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

Simple database for ssl tests
Attachment #8477688 - Flags: review?(ryan.sleevi)
Attachment #8478654 - Flags: review?(rrelyea)
Comment on attachment 8477695 [details] [diff] [review]
test-code

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

Following Google style
Attachment #8477695 - Flags: review?(wtc)
Comment on attachment 8477687 [details] [diff] [review]
add-gtest-framework

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

It's not clear where you plan on landing this, but maybe I'm just bad with Splinter.

I do think it's weird to have gtests/gtest (which then has a subdir gtest, but that part I totally get is necessary). I'm not sure who best to get feedback from on the desired structure, but I suspect Kai might have opinions/ideas on this. I do note that it's consistent with freebl/mpi (where mpi is a 'third party' library, at least in theory), so it may entirely be appropriate to have tests/gtests/gtest. Or perhaps it makes sense to have tests/gtest (which holds gtest in tests/gtest/gtest) and then have individual gtests just in tests/.

Conceptually though, it works for me either way, but I'm setting it to f+ rather than r+.

You could also have it strip the unnecessary build configs from the import, but I think it's cleaner and clearer to do what you did (which is include all the upstream files, even if unused by NSS)
Attachment #8477687 - Flags: review?(ryan.sleevi)
Attachment #8477687 - Flags: review?(kaie)
Attachment #8477687 - Flags: feedback+
Comment on attachment 8477688 [details] [diff] [review]
add-gtest-db

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

I'm not a fan of checking in binary files for these SSL tests.

Based on our experience in Chrome, I'd much rather see them autogenerated by a test script, which makes it much easier to reason about (examine the script) and change (as necessary)
Attachment #8477688 - Flags: review?(ryan.sleevi) → review-
(In reply to Ryan Sleevi from comment #10)
> Comment on attachment 8477688 [details] [diff] [review]
> add-gtest-db
> 
> Review of attachment 8477688 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not a fan of checking in binary files for these SSL tests.
> 
> Based on our experience in Chrome, I'd much rather see them autogenerated by
> a test script, which makes it much easier to reason about (examine the
> script) and change (as necessary)

Thanks for your review. I agree that the generator is necessary. But to simplify
testing (and in lieu of other nss dirs) is it OK with you to bundle the database and
the generator(in a patch) or do you prefer only the generator as the patch.
(In reply to Camilo Viecco (:cviecco) from comment #11)
> (In reply to Ryan Sleevi from comment #10)
> > Comment on attachment 8477688 [details] [diff] [review]
> > add-gtest-db
> > 
> > Review of attachment 8477688 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I'm not a fan of checking in binary files for these SSL tests.
> > 
> > Based on our experience in Chrome, I'd much rather see them autogenerated by
> > a test script, which makes it much easier to reason about (examine the
> > script) and change (as necessary)
> 
> Thanks for your review. I agree that the generator is necessary. But to
> simplify
> testing (and in lieu of other nss dirs) is it OK with you to bundle the
> database and
> the generator(in a patch) or do you prefer only the generator as the patch.

I'm not sure what you meant of "in lieu of other NSS dirs".

I'd much prefer only the patch and the DBs be generated as part of the test invocation. What's the concern here?

Checking in a test DB is just a recipe for a dirty tree. And we generate test DBs all the time in our other tests as part of the test scripts.
Attached patch add-gendb-script (obsolete) — Splinter Review
Attachment #8477688 - Attachment is obsolete: true
Attachment #8481568 - Flags: review?(ryan.sleevi)
(In reply to Ryan Sleevi from comment #12)
> (In reply to Camilo Viecco (:cviecco) from comment #11)
> > (In reply to Ryan Sleevi from comment #10)
> > > Comment on attachment 8477688 [details] [diff] [review]
> > > add-gtest-db
> > > 
> > > Review of attachment 8477688 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > I'm not a fan of checking in binary files for these SSL tests.
> > > 
> > > Based on our experience in Chrome, I'd much rather see them autogenerated by
> > > a test script, which makes it much easier to reason about (examine the
> > > script) and change (as necessary)
> > 
> > Thanks for your review. I agree that the generator is necessary. But to
> > simplify
> > testing (and in lieu of other nss dirs) is it OK with you to bundle the
> > database and
> > the generator(in a patch) or do you prefer only the generator as the patch.
> 
> I'm not sure what you meant of "in lieu of other NSS dirs".
> 
> I'd much prefer only the patch and the DBs be generated as part of the test
> invocation. What's the concern here?

> 
> Checking in a test DB is just a recipe for a dirty tree. And we generate
> test DBs all the time in our other tests as part of the test scripts.

Agreed, new patch, just generator.
Comment on attachment 8481568 [details] [diff] [review]
add-gendb-script

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

::: gtests/ssl/gen-db.sh
@@ +8,5 @@
> +cd data
> +dd if=/dev/urandom bs=512 count=1 of=noise
> +echo "" > pwfile
> +
> +certutil -d . -N -f pwfile

Shouldn't this output to a test directory, for that iteration, similar to all the other NSS unit tests?

That is, I'm having a very hard time seeing what your overall "vision" is for how tests run. In Chrome, for example, tests always run from an output directory (out/Debug / out/Release, etc), and if they create any files, it's in explicit temporary directories that get created by and destroyed by the GTest under execution (e.g. on the C++ side)

In NSS, all tests (literally, all.sh) is responsible for setting up a temporary directory *outside* of both the source and build directories to hold test artifacts, where it then outputs these tests. You can see the use of the ${DB} variable in the .sh files used.

Rather than focusing too much on this specific .sh, I think it may help me (and other reviewers) if you focus on where your "end goal" is for how you see things working, and then we can work back from that to see how close these scripts get to the mark, or what are the transition phases to that end goal.

For now, since this would have the effect of modifying the source tree (which I tried to be clear was a bad thing in my previous comment), r-
Attachment #8481568 - Flags: review?(ryan.sleevi) → review-
Comment on attachment 8481568 [details] [diff] [review]
add-gendb-script

On which platforms do you intend to run these tests? Because currently, the NSS tests run everywhere (except the memory leak tests, which require valgrind).

I guess you haven't tried to run it on windows yet, because your ssl/gen-db.sh code calls "dd if=/dev/urandom" which I don't think will run on windows?

Have a look at tests/common/init.sh, the code that creates the tests_noise file.

Why don't you integrate your tests as an optional step of the existing tests? Then you could benefit from the existing tests_noise file, and other things that are already created which might you could copy/reuse.

Note that we added certutil -N --empty-password a while ago, so you could avoid pwfile.
search for BUILD_LIBPKIX_TESTS as an example for optional tests, that are enabled/disabled based on an environment, and integrated into the existing NSS test system.
Kai, thank you for your review.

(In reply to Kai Engert (:kaie) from comment #16)
> Comment on attachment 8481568 [details] [diff] [review]
> add-gendb-script
> 
> On which platforms do you intend to run these tests? Because currently, the
> NSS tests run everywhere (except the memory leak tests, which require
> valgrind).

I think these tests should be able to run everywhere.


> I guess you haven't tried to run it on windows yet, because your
> ssl/gen-db.sh code calls "dd if=/dev/urandom" which I don't think will run
> on windows?
> 
> Have a look at tests/common/init.sh, the code that creates the tests_noise
> file.
> 
> Why don't you integrate your tests as an optional step of the existing
> tests? Then you could benefit from the existing tests_noise file, and other
> things that are already created which might you could copy/reuse.

Will this allow the cert db to stick around after the tests complete?
It would be convenient for that to be the case, since then we can
use it for manual tests.
I see that you added tests that are completely separate from the existing test. I think it would be good to integrate. I'll give you some guidance to the existing tests, then you can explore if you really want to be completely separate, or if integrating is easy.

Have you looked at the existing tests in nss/tests/all.sh ?

You could add a new category of tests. In case you wonder, it's possible to run only your new category of tests, by setting environment variables. For example, 
  export NSS_CYCLES=standard
says, only cycle once through the NSS tests (skip the libpkix/shared-db variations),
and
  export NSS_TESTS=cert
says, only run the "cert" subtest.

If you execute the above, you can see which standard files are already being created, which you could use as base files for your new tests.

For example, I have recently added new tests for OCSP stapling. I extended the base cert.sh scripts to produce additional files used for the stapling tests.

Search for "stapl" in common/init.sh and cert/cert.sh, to see how I simply copy one of the other directories into a new stapling directory, to be used by the recently added stapling tests.

(also I added this directory to a list in dbupgrade.sh for completeness)



You could call your new category "gtf" for "tests that depend on the google test framework".

Look at tests/ocsp/ocsp.sh for a minimal script to drive a category of tests.
(In reply to Eric Rescorla (:ekr) from comment #18)
> Kai, thank you for your review.

It's not a full review yet, I tend to add multiple comments as I walk along...

 
> > Why don't you integrate your tests as an optional step of the existing
> > tests? Then you could benefit from the existing tests_noise file, and other
> > things that are already created which might you could copy/reuse.
> 
> Will this allow the cert db to stick around after the tests complete?
> It would be convenient for that to be the case, since then we can
> use it for manual tests.

Yes, whenever you run the existing NSS tests, there is an output directory created (one level above the nspr/nss directories). It's named tests_results. Each time you run the tests again, an additional subdirectory below tests_results will be created.

If you have run the NSS tests before:

Set at least the 
  export USE_64=1 
environment variable.

If your local hostname isn't in DNS, then set:
  export HOST=localhost
  export DOMSUF=localdomain

(assuming that ping localhost.localdomain works on your machine).

Then 
  cd nss/tests
  ./all.sh

You'll see a full logfile below tests_results/
You can follow output.log as the tests run.

You can grep case-sensitive for PASSED or FAILED for result lines in the file, e.g. 
  tail -f output.log | egrep "PASSED|FAILED" 
to watch the tests proceed.
I notice that gtfw has a license different from MPL2. It requires attribution in binaries - something that MPL2 doesn't require. People who ever package test binaries need to be aware of this special situation.

We should involve gerv@m.o into the process of getting a copy of gtfw added to NSS.


I'd prefer something like "gtfw" over gtest (since a simple "g" seems too generic).

If possible, it would be good to have all new tests scripts added below the existing directories whenever possible.

However, I understand that the nss/tests/ directory contains only scripts, but nothing is compiled. But the gtfw must be compiled, so having it as a new top level directory seems reasonable.

But to make it clear that gtfw has a separate license, maybe we should have a new top level directory for external, optional code, that's clearly named as such.

How about
  nss/other_licenses/gtfw

In other words, I suggest to move
  gtests/gtest/gtest -> other_licenses/gtfw

I'd also propose to add a file
  other_licenses/readme.gtfw
which should mention what gtfw is, and where it's from (url).


Next, you are adding a binary for testing. Usually, binary utilities all live in nss/cmd.

I'd suggest your new tool should be named to indicate that it requires the gtfw, primarily because of the separate licensing issue (I don't know if I worry to much and that's overkill). How about changing your generic "ssl_unittests" name to something similar to "ssl_gtfw_test"?


As explained in the previous comments, we'll need a new script to drive your new test. You could name the new category (which is added to all.sh) "ssl_gtfw".

You'd require a new directory
  nss/tests/ssl_gtfw
and a new script
  nss/tests/ssl_gtfw.sh
which can be very simply, as explained above, see ocsp.h, plus the necessary additions to all.sh

If you want to use the output from cert.sh (or your own additions to it), then run cert.sh from within your ssl_gtfw.sh.

Then simply run whatever is necessary to execute the tests, e.g. call your binary utility.

You should check the exit status of your tool, and either print PASSED or FAILED to standard output.
Everyone cals it gtest or googletest. No need to make a new acronym (gtfw)
Kai, thanks for the reviews and great suggestions. My initial plan was to have these be optional focusing on *nix systems, but I can see that was just deficient. So new vision:

1. Tests will run from all platforms. (seems in agrement with everyone here so far)
2. The tests must be integrated with the current test infrastructure and should generate any neccesary data in an directory to be deleted by the test infrastructure, the scripts should also we able to generate temporary data in a specified directory to facilitate manual testing. (covers Ryan, Kai and Eric's concerns).
3. Mozilla-central already includes gtest, so while I dont think is a big issue I will contact gerv@m.o to ask him about this.
4. The naming convention for the generated tests should be changed so that includes gtest as part of its name (Kaie, is that are you comfortable or you prefer googletest as the term to be included). 
5. Given 4 and the suggestion in comment 21:
new directory:
 nss/tests/ssl_gtests
and new script
 nss/tests/ssl_gtests.sh

The binary should also be renamed to:
ssl_gtest_test

Any issue that anybody thinks I am missing?
Comment on attachment 8478654 [details] [diff] [review]
build-system-patches (v2)

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

Bob, instead of reviewing, could you take a look at comment 23 (https://bugzilla.mozilla.org/show_bug.cgi?id=1057584#c23) and see if it aligns with you.
Attachment #8478654 - Flags: review?(rrelyea)
(In reply to Camilo Viecco (:cviecco) from comment #23)
> Kai, thanks for the reviews and great suggestions. My initial plan was to
> have these be optional focusing on *nix systems, but I can see that was just
> deficient. So new vision:

These pretty much look good to me, modulo one thing:

> 1. Tests will run from all platforms. (seems in agrement with everyone here
> so far)

This seems desirable, but low-priority.  If there are significant impediments on one or more platforms, I would prefer to go ahead and land something that works on some platforms and fails cleanly on others.  Then handle the other platforms in a separate follow-up bug.
If you are importing this stuff into a new directory, then you should add a LICENSE file at the top level with a copy of the BSD license that it uses.

If the code ships with Firefox, you need to file a bug to have the text added to about:license (unless it's already there, which it may well be for Google's variant of BSD.)

Gerv
Attachment #8481568 - Attachment is obsolete: true
Attachment #8483123 - Attachment is obsolete: true
Attached patch test-code (v1.1) (obsolete) — Splinter Review
Same code as before, but now with agreed naming convention for the output file. (also updated documentation to reflect integration with NSS test suite).
Comment on attachment 8477695 [details] [diff] [review]
test-code

Obsoleted by 1.1 (which is just a rename of the output and a cleanup of the readme)
Attachment #8477695 - Attachment is obsolete: true
Attachment #8477695 - Flags: review?(wtc)
Attachment #8483592 - Flags: review?(wtc)
Attached patch hook-new-tests-to-nss-test-suite (obsolete) — Splinter Review
Attachment #8483562 - Attachment is obsolete: true
Attached patch build-system-patches (v3) (obsolete) — Splinter Review
Attachment #8478654 - Attachment is obsolete: true
Attachment #8483597 - Flags: review?(kaie)
(In reply to Gervase Markham [:gerv] from comment #26)
> If you are importing this stuff into a new directory, then you should add a
> LICENSE file at the top level with a copy of the BSD license that it uses.
> 
> If the code ships with Firefox, you need to file a bug to have the text
> added to about:license (unless it's already there, which it may well be for
> Google's variant of BSD.)

It's there: about:license#google-bsd
Why do you use gtests/gtest/gtest ?

Would it work to make it one level shorter, gtests/gtest ?
Gtests/gest/gtest is conceptually the same as nss/lib/freebl

The first is the overall directory structure
The second level is the "this is the third party code, checked in, unmodified"
The third level is something that the third party library has done

Having gtests/gtest implies the third party google test is checked directly into the root of the gtests Dir

Alternatively, it could be

[Gtests/google_test]/{gtest}

Where [] denotes NSS controlled structure, and {} is the third party structure.
Comment on attachment 8483598 [details] [diff] [review]
build-system-patches (v3)

This works on *nix systems. I still need to submit the visual studio linker for windows.
Attachment #8483598 - Flags: feedback?(rrelyea)
(In reply to Ryan Sleevi from comment #35)
> Gtests/gest/gtest is conceptually the same as nss/lib/freebl

Thanks for your explanations. In particular I like the idea of using different names for the 2nd and 3rd level.

There is no nss directory. It starts with the "lib" hierarchy. In your comparison, the created hierarchy is "nss/gtests/gtest/gtest".

The first gtests makes sense to me. It's the host for everything related to, based on the google test framework.

The second level, gtests/gtest, currently contains makefiles only, for driving the build.

The third level, gtests/gtest/gtest, contains only the imported code, without modifications.

I see that by having the additional level, on top of the import code, allows you to avoid placing any makefiles inside it. This makes sense, being able to completely replacing the imported directory, without having to mess inside it.

So, the primary thing that I dislike, is that these three directory names are nearly identical.

This is of course bikeshedding, but maybe it would be helpful to have slightly more explanatory directory names.

I would prefer the top level directory to be something like:
  external_tests

The second level, which could potentially contain multiple external test frameworks:
  google_test       <- our makefiles
  google_test/gtest <- the unchanged imported code

and
  ssl_gtest   <- our test application that requires gtest


In other words, my new suggestion is:

- keep the number of directory levels
- rename /gtest to /external_tests
- rename /gtests/gtest to /external_tests/google_test
- rename /gtests/gtest/gtest to /external_tests/google_test/gtest
- rename /gtests/ssl to /gtests to /external_tests/ssl_gtest
If you can think of a better name than /external_tests that would be fine, too, but it would be nice to have a name that's more generic.
Comment on attachment 8483597 [details] [diff] [review]
hook-new-tests-to-nss-test-suite

>+cert_ssl_gtests()
>+{
>+  CERTFAILED=0
>+  echo "$SCRIPTNAME: Creating ssl_gtest DB dir"
>+  cert_init_cert ${SSLGTESTDIR} "server" 1 ${D_EXT_SERVER}
>+  echo "$SCRIPTNAME: Creating database for ssl_gtests"
>+  certu -N -d "${SSLGTESTDIR}" --empty-password 2>&1


I don't understand the order here. Shouldn't the init "certu -N" command run first?

Or, if the first command already creates the DB that you require, then the second command is unnecessary?
(In reply to Kai Engert (:kaie) from comment #39)
> Comment on attachment 8483597 [details] [diff] [review]
> hook-new-tests-to-nss-test-suite
> 
> >+cert_ssl_gtests()
> >+{
> >+  CERTFAILED=0
> >+  echo "$SCRIPTNAME: Creating ssl_gtest DB dir"
> >+  cert_init_cert ${SSLGTESTDIR} "server" 1 ${D_EXT_SERVER}
> >+  echo "$SCRIPTNAME: Creating database for ssl_gtests"
> >+  certu -N -d "${SSLGTESTDIR}" --empty-password 2>&1
> 
> 
> I don't understand the order here. Shouldn't the init "certu -N" command run
> first?
> 
> Or, if the first command already creates the DB that you require, then the
> second command is unnecessary?

The cert_init_cert name  is really misleading, it does not create the db
but just creates the directory and sets up some environment variables. I use
it because other places in the code used that scheme. (I can replace by a mkdir -p)
I applied all your patches and ran the test suite. It passed without errors, however, your new test isn't integrated into the final summary that gets printed.

It reports a total of #397 tests, but doesn't complete your test afterwards. My previous recommendation to print PASSED or FAILED wasn't good enough.

There are helper functions that you can call, in order to print a final status of your test result.

In your nss/tests/ssl_gtests/ssl_gtests.sh you can call html_msg, which takes four parameters. Are you able to distinguish success from failure based on the process exit status of your binary ssl_gtest_test? That's the usual approach to detect success/failure.

After you run your binary, call html_msg like this:

  html_msg $? expected-exit-code "name of your test"

$? is the process exit code from the most recent commant.

expected-exit-code is the process exit code that your binary returns (using "return" in the main function, or using the exit() function).

When you call html_msg, it will compare the actual exit code against the expected code, will print either PASSED or FAILURE into the log file, and will include the test in the summary.
(In reply to Camilo Viecco (:cviecco) from comment #40)
> The cert_init_cert name  is really misleading, it does not create the db
> but just creates the directory and sets up some environment variables. I use
> it because other places in the code used that scheme. (I can replace by a
> mkdir -p)

Thanks for the explanation. No need to change if it works.
Comment on attachment 8483592 [details] [diff] [review]
test-code (v1.1)

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

r=wtc.

The code in this patch conforms to the Google C++ Style Guide at
least 80%, so I'm assuming that the intention is to follow that
style guide. Most of my suggested changes are based on that
assumption.

I also recommend we figure out how to run the clang format tool
so that we don't need to debate formatting issues.

::: gtests/ssl/databuffer.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Original author: ekr@rtfm.com

Nit: let's omit this kind of author comment. It's hard to maintain.

@@ +11,5 @@
> +
> +class DataBuffer {
> + public:
> +  DataBuffer() : data_(nullptr), len_(0) {}
> +  DataBuffer(const uint8_t *data, size_t len)

Nit: in C++ code let's put the '*' next to the type. Please check the
entire patch.

@@ +27,5 @@
> +  }
> +
> +  void Allocate(size_t len) {
> +    delete[] data_;
> +    data_ = new unsigned char[ len ? len : 1];  // Don't depend on new [0].

Nit: don't need a space after '['.

::: gtests/ssl/gtest_utils.h
@@ +50,5 @@
> +} while(0)
> +
> +#define ASSERT_TRUE_WAIT(expression, timeout) \
> +  do { \
> +        WAIT_(expression,timeout); \

Nit: add a space after ','.

::: gtests/ssl/ssl_gtest_test.cc
@@ +12,5 @@
> +  // Start the tests
> +  ::testing::InitGoogleTest(&argc, argv);
> +  std::string path = ".";
> +
> +  for (int i = 0; i<argc; i++) {

Nit: add a space before and after '<'.

@@ +20,5 @@
> +    }
> +  }
> +
> +  NSS_Initialize(path.c_str(),
> +    "", "", SECMOD_DB, NSS_INIT_READONLY);

Nit: four spaces indentation for line continuation. Alternatively,
you can align with the first argument.

::: gtests/ssl/ssl_loopback_unittest.cc
@@ +4,5 @@
> +#include <pk11func.h>
> +#include <ssl.h>
> +#include <sslerr.h>
> +#include <sslproto.h>
> +#include <keyhi.h>

Nit: use "foo.h" instead of <foo.h> for NSPR and NSS headers.

@@ +27,5 @@
> +  virtual void Inspect(DummyPrSocket* adapter,
> +                       const void* data, size_t len) {
> +    TlsRecordParser parser(static_cast<const unsigned char *>(data), len);
> +
> +    uint8_t ct;

Nit: what does 'ct' stand for? This variable is too short and
not an obvious abbreviation.

@@ +46,5 @@
> +// DTLS records of various types.
> +class TlsInspectorInjector : public TlsRecordInspector {
> + public:
> +  TlsInspectorInjector(uint8_t packet_type, uint8_t handshake_type,
> +                    const unsigned char *data, size_t len) :

Nit: indentation is off.

@@ +212,5 @@
> +  enum Role { CLIENT, SERVER};
> +  enum State { INIT, CONNECTING, CONNECTED, ERROR };
> +
> +  TlsAgent(const std::string& name, Role role, Mode mode) :
> +      name_(name),

Nit: in the Google style guide, the colon should be placed
on this line, with four spaces indentation, followed by
the initializers:

  TlsAgent(const std::string& name, Role role, Mode mode)
      : name_(name),
        mode_(mode),
        pr_fd_(nullptr),
        ...

::: gtests/ssl/test_io.cc
@@ +21,5 @@
> +namespace nss_test {
> +
> +static PRDescIdentity test_fd_identity = PR_INVALID_IO_LAYER;
> +
> +#define UNIMPLEMENTED                                           \

Nit: this should be moved to a utility header file. We can
do this later.

Please define this macro to look like a function (with no arguments):

#define UNIMPLEMENTED()                                     \

::: gtests/ssl/test_io.h
@@ +28,5 @@
> +  virtual void Inspect(DummyPrSocket* adapter,
> +                       const void *data, size_t len) = 0;
> +};
> +
> +enum Mode { STREAM, DGRAM};

Nit: add a space before '}'.
Attachment #8483592 - Flags: review?(wtc) → review+
Wan-Teh,

Thanks for your review. The intent is indeed to follow the Google style.

I'm not sure about how to run clang style to fix this up (MT may know).

Camilo: you might be able to get some of the way here with the style
by loading it into Emacs with google-style.el and then doing indent-region
over the entire file. That should clean up most of the indentation problems
that come from name changes, pilot error, etc.
s/clang style/clang format/

http://clang.llvm.org/docs/ClangFormat.html
Attached patch test-code (v1.2) (obsolete) — Splinter Review
Keeping r+ from wtc
New files after cleanup via 'clang-format -style=Google'
Attachment #8485247 - Flags: review+
Attached patch test-code (v1.3) (obsolete) — Splinter Review
keeping r+ from wtc. Addresed manually non format issued and then passed by clang-format =style=Google to autoformat nits.
Attachment #8483592 - Attachment is obsolete: true
Attachment #8485247 - Attachment is obsolete: true
Attachment #8485273 - Flags: review+
Attached patch adding-gtest-to-test-counter (obsolete) — Splinter Review
Addressing comment 41 (https://bugzilla.mozilla.org/show_bug.cgi?id=1057584#c41) for hook-new-tests-to-nss-test-suite.
> and
>   ssl_gtest   <- our test application that requires gtest
> 
> 
> In other words, my new suggestion is:
> 
> - keep the number of directory levels
> - rename /gtest to /external_tests
> - rename /gtests/gtest to /external_tests/google_test
> - rename /gtests/gtest/gtest to /external_tests/google_test/gtest
> - rename /gtests/ssl to /gtests to /external_tests/ssl_gtest

OK will rename to be this way. Thank you
Thank you for hooking up the test to the counter.

My primary concern is using a good name for the top level directory.
If you rename the directories like I suggested in comment 37, r=kaie for adding all your test patches.

I'd have prefered if your own test utility lived in nss/cmd/ - but given that it depends on the external code, I'm fine keeping it separately.

Since the tests are built optionally, everything else can be fixed later.
Comment on attachment 8483598 [details] [diff] [review]
build-system-patches (v3)

r+, though please make sure .cpp actually continues to work with this syntax before you check in.
Attachment #8483598 - Flags: feedback?(rrelyea) → feedback+
r+ from Kaie after renaming agreed in https://bugzilla.mozilla.org/show_bug.cgi?id=1057584#c37
Attachment #8477687 - Attachment is obsolete: true
Attachment #8477687 - Flags: review?(kaie)
Attachment #8485962 - Flags: review+
keeping r+ from rrelyea post checking cpp files still compile (under linux), also with renaming convention as comment 37
Attachment #8483598 - Attachment is obsolete: true
Keeping r+ from Kaie post renaming and integrating counter
Attachment #8483597 - Attachment is obsolete: true
Attachment #8485284 - Attachment is obsolete: true
Attachment #8483597 - Flags: review?(kaie)
Attachment #8485966 - Flags: review+
Attached patch test-code (v1.4)Splinter Review
Post renaming as suggested in comment 37. Keeping r+ from wtc
Attachment #8485273 - Attachment is obsolete: true
Attachment #8485968 - Flags: review+
Pushed to NSS:
changeset:   11249:c8358d66ca3e
tag:         tip
user:        Camilo Viecco <cviecco@mozilla.com>
date:        Mon Sep 08 13:09:40 2014 -0700
summary:     Bug 1057584 - Add gtest framework and initial tests optionally (4/4). Hooks with test suite. r=kaie

changeset:   11248:2f4676c16d52
user:        Camilo Viecco <cviecco@mozilla.com>
date:        Mon Sep 08 13:09:40 2014 -0700
summary:     Bug 1057584 - Add gtest framework and initial tests optionally (3/4). Testing Code. r=wtc

changeset:   11247:887dabda1c76
user:        Camilo Viecco <cviecco@mozilla.com>
date:        Mon Sep 08 13:09:40 2014 -0700
summary:     Bug 1057584 - Add gtest framework and initial tests optionally (2/4). Build Changes. r=rrelyea

changeset:   11246:e2e24aa37985
user:        Camilo Viecco <cviecco@mozilla.com>
date:        Mon Sep 08 13:09:40 2014 -0700
summary:     Bug 1057584 - Add gtest framework and initial tests optionally (1/4). Add framework. r=kaie
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.