1244309 - Integrate the libnssutil UTF-8 tests into some test framework

Status: RESOLVED FIXED

Description

[Jed Davis [:jld] (⏰UTC-6)]

lib/util/utf8.c has a bunch of test code that we're apparently not using, but it still more or less works.  The build works by compiling utf8.c with an ifdef that enables the test code and a main function to make a standalone executable (similar to the NSPR tests), but it's not hooked up to the usual shell scripts.

This could be wrapped in a shell script, but I think we're trying to do less of that.  Transcribing everything into gtests might or might not be less work than writing new tests from scratch.

One thing that needs to happen first (at least if the code is going to be used mostly as-is, rather than partially or entirely rewritten in C++) is to fix the many -Werror failures; currently it needs to be built with explicit NSS_ENABLE_WERROR=0.

Comment 1

[Jed Davis [:jld] (⏰UTC-6)]

Created attachment 8721106 [details] [diff] [review]
WIP, part 1: add gtests

Work in progress.  This adds the nssutil_gtest command, but doesn't have scripting to tie it into all.sh (yet).  Also, I just copied the test data from the old C code, so the identifier naming if nothing else is almost certainly not in conformance with our C++ style.  And I haven't tried to reproduce the “multichar” test, which just concatenated all the test characters together in order.  (It might be better to try every pair of characters or something like that.)

Comment 2

[Jed Davis [:jld] (⏰UTC-6)]

Created attachment 8721107 [details] [diff] [review]
WIP, part 2: remove old tests

Comment 3

[Jed Davis [:jld] (⏰UTC-6)]

Created attachment 8722318 [details] [diff] [review]
WIP, part 1: add gtests [v2]

Added a wrapper script (but it's kind of slow due to bug 1250304) and renamed from "nssutil_gtest" to "util_gtest" (to match that the lib is in lib/util).  Still need to fix the inherited-from-C-code style violations.

Comment 4

[Jed Davis [:jld] (⏰UTC-6)]

Created attachment 8723284 [details] [diff] [review]
Patch, part 1: add gtests

Comment 5

[Jed Davis [:jld] (⏰UTC-6)]

Created attachment 8723285 [details] [diff] [review]
Patch, part 2: remove old tests

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8723284 [details] [diff] [review]
Patch, part 1: add gtests

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

This is great work, thank you. It looks mostly good but with C++ at hand we should seize the chance and clean it up a little, and modernize it.

Also, can you add util_gtests to circle.yml as well?

::: external_tests/util_gtest/Makefile
@@ +30,5 @@
> +CFLAGS += -I$(CORE_DEPTH)/lib/util
> +
> +ifdef NSS_SSL_ENABLE_ZLIB
> +include $(CORE_DEPTH)/coreconf/zlib.mk
> +endif

We might not actually need this?

::: external_tests/util_gtest/manifest.mn
@@ +1,1 @@
> +# -*- makefile -*-

I think this is wrong?

::: external_tests/util_gtest/util_gtest.cc
@@ +1,2 @@
> +#include "nspr.h"
> +#include "prenv.h"

Don't need this with g_working_dir_path removed.

@@ +5,5 @@
> +
> +#define GTEST_HAS_RTTI 0
> +#include "gtest/gtest.h"
> +
> +std::string g_working_dir_path;

This seems unused? I think calling only RUN_ALL_TESTS() is sufficient.

@@ +25,5 @@
> +  }
> +
> +  int rv = RUN_ALL_TESTS();
> +
> +  return rv;

return RUN_ALL_TESTS();

::: external_tests/util_gtest/util_utf8_unittest.cc
@@ +17,5 @@
> +// Structures to represent test cases.  These are small enough that
> +// passing by value isn't a problem.
> +
> +struct Ucs4Case {
> +  PRUint32 c;

Please s/PRUint32/uint32_t in this file.

@@ +18,5 @@
> +// passing by value isn't a problem.
> +
> +struct Ucs4Case {
> +  PRUint32 c;
> +  const char *utf8;

uint8_t*

@@ +22,5 @@
> +  const char *utf8;
> +};
> +
> +struct Ucs2Case {
> +  PRUint16 c;

uint16_t (and so on... :)

@@ +51,5 @@
> +  { 0x00000020, "\x20" },
> +  { 0x0000003F, "\x3F" },
> +  { 0x00000040, "\x40" },
> +  { 0x0000007F, "\x7F" },
> +          

Nit: remove white space

@@ +94,5 @@
> +  { 0x00000480, "\xD2\x80" },
> +  { 0x00000500, "\xD4\x80" },
> +  { 0x00000600, "\xD8\x80" },
> +  { 0x000007FF, "\xDF\xBF" },
> +          

Nit: remove white space

@@ +170,5 @@
> +  { 0x00009000, "\xE9\x80\x80" },
> +  { 0x0000A000, "\xEA\x80\x80" },
> +  { 0x0000C000, "\xEC\x80\x80" },
> +  { 0x0000FFFF, "\xEF\xBF\xBF" },
> +          

Nit: remove white space

@@ +285,5 @@
> +  { 0x0020, "\x20" },
> +  { 0x003F, "\x3F" },
> +  { 0x0040, "\x40" },
> +  { 0x007F, "\x7F" },
> +          

Nit: remove white space

@@ +331,5 @@
> +  { 0x0480, "\xD2\x80" },
> +  { 0x0500, "\xD4\x80" },
> +  { 0x0600, "\xD8\x80" },
> +  { 0x07FF, "\xDF\xBF" },
> +          

Nit: remove white space

@@ +723,5 @@
> +// Tests of sec_port_ucs4_utf8_conversion_function, by itself, on
> +// valid inputs:
> +
> +class Ucs4Test : public ::testing::TestWithParam<Ucs4Case> {
> +};

As with the ssl_gtests files, can we collect test class definitions at the top?

@@ +732,5 @@
> +  unsigned int len = 0;
> +  PRBool result;
> +  const Ucs4Case testCase = GetParam();
> +
> +  memset(utf8, 0, sizeof(utf8));

We can remove the memset if above we do unsigned char utf8[8] = {0};

@@ +733,5 @@
> +  PRBool result;
> +  const Ucs4Case testCase = GetParam();
> +
> +  memset(utf8, 0, sizeof(utf8));
> +  nc = htonl(testCase.c);

At the top, we could do:

TEST_P(Ucs4Test, ToUtf8) {
  const Ucs4Case testCase = GetParam();
  PRUint32 nc = htonl(testCase.c);
  ...

@@ +738,5 @@
> +
> +  result = sec_port_ucs4_utf8_conversion_function(PR_FALSE,
> +    (unsigned char *)&nc, sizeof(nc), utf8, sizeof(utf8), &len);
> +
> +  ASSERT_EQ(PR_TRUE, result);

ASSERT_TRUE(), and everywhere else.

@@ +741,5 @@
> +
> +  ASSERT_EQ(PR_TRUE, result);
> +  ASSERT_LT(len, sizeof(utf8));
> +  EXPECT_EQ(std::string(testCase.utf8), std::string((char *)utf8, len));
> +  EXPECT_EQ('\0', utf8[len]);

I think we should probably stick to either ASSERT_* or EXPECT_*, they seem to have the same implementations, right?

@@ +747,5 @@
> +
> +TEST_P(Ucs4Test, FromUtf8) {
> +  PRUint32 nc;
> +  unsigned int len = 0;
> +  PRBool result;

bool result;

@@ +774,5 @@
> +
> +  result = sec_port_ucs4_utf8_conversion_function(PR_FALSE,
> +    (unsigned char *)&nc, sizeof(nc), utf8end - len, len, &len);
> +
> +  ASSERT_EQ(result, PR_FALSE);

ASSERT_FALSE()

@@ +779,5 @@
> +  ASSERT_EQ(strlen(testCase.utf8), len);
> +}
> +
> +INSTANTIATE_TEST_CASE_P(Ucs4TestCases, Ucs4Test,
> +                        ::testing::ValuesIn(kUcs4Cases));

And can we group these macro calls at the bottom?

@@ +794,5 @@
> +  unsigned int len = 0;
> +  PRBool result;
> +  const Ucs2Case testCase = GetParam();
> +
> +  memset(utf8, 0, sizeof(utf8));

unsigned char utf8[8] = {0};

@@ +939,5 @@
> +};
> +
> +TEST_P(BadUtf8Test, HasNoUcs2) {
> +  PRBool result;
> +  unsigned char destBuf[30]; // ??? length copied from original C

I assume a "big" length was chosen to ensure we won't fail due to |if( len > maxOutBufLen ) {|.

@@ +975,5 @@
> +  Utf16BadCase srcBuf;
> +  unsigned char destBuf[18];
> +  unsigned int len;
> +  const Utf16BadCase testCase = GetParam();
> +  static const size_t maxLen = sizeof(srcBuf.w) / sizeof(srcBuf.w[0]);

I think we could do:

static const size_t maxLen = PR_ARRAY_SIZE(srcBuf.w);

@@ +1006,5 @@
> +  unsigned char utf8[3];
> +  unsigned int len = 0;
> +  const Ucs2Case testCase = GetParam();
> +
> +  memset(utf8, 0, sizeof(utf8));

unsigned char utf8[3] = {0};

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

Do we need to add this here? None of the other *_gtests is in here.

Comment 7

[Jed Davis [:jld] (⏰UTC-6)]

(In reply to Tim Taubert [:ttaubert] from comment #6)
> This is great work, thank you. It looks mostly good but with C++ at hand we
> should seize the chance and clean it up a little, and modernize it.
> 
> Also, can you add util_gtests to circle.yml as well?

Nice; I hadn't noticed that.  Done.

> ::: external_tests/util_gtest/Makefile
> @@ +30,5 @@
> > +ifdef NSS_SSL_ENABLE_ZLIB
> > +include $(CORE_DEPTH)/coreconf/zlib.mk
> > +endif
> 
> We might not actually need this?

Thanks; fixed.

> ::: external_tests/util_gtest/manifest.mn
> @@ +1,1 @@
> > +# -*- makefile -*-
> 
> I think this is wrong?

How so?  That there's an emacs mode line there at all, or the way it's formatted, or...?  I can see the point that marking up manifest.mn files like that ought to be done across the tree and as a separate bug if it's to be done at all.

> ::: external_tests/util_gtest/util_gtest.cc
> @@ +1,2 @@
> > +#include "nspr.h"
> > +#include "prenv.h"
> 
> Don't need this with g_working_dir_path removed.
> 
> @@ +5,5 @@
> > +std::string g_working_dir_path;
> 
> This seems unused? I think calling only RUN_ALL_TESTS() is sufficient.

Indeed; that's left over from when I wasn't sure how much of the main function I copied I actually need.  I'll GC all of that.

> @@ +25,5 @@
> > +  int rv = RUN_ALL_TESTS();
> > +
> > +  return rv;
> 
> return RUN_ALL_TESTS();

...and that.

> ::: external_tests/util_gtest/util_utf8_unittest.cc
> @@ +17,5 @@
> > +struct Ucs4Case {
> > +  PRUint32 c;
> 
> Please s/PRUint32/uint32_t in this file.
>
> @@ +22,5 @@
> > +struct Ucs2Case {
> > +  PRUint16 c;
> 
> uint16_t (and so on... :)

I'd left them that way to match the NSS interfaces, but these are being passed through hton[ls] anyway.  They seem to be the same types on the systems I have available, at least.  (I'm willing to assume that uint32_t and PRUint32 are bit-representation-equivalent, but something like 32-bit int vs. 32-bit long could cause a build failure, so I wanted to check.)

> @@ +18,5 @@
> > +struct Ucs4Case {
> > +  PRUint32 c;
> > +  const char *utf8;
> 
> uint8_t*

That breaks everything: unsigned char * can't be initialized with a string literal.  (Among other problems.)  I could cast them all, I guess, but... is that really an improvement?

> @@ +51,5 @@
> > +          
> 
> Nit: remove white space
> 
> @@ +94,5 @@
> > +          
> 
> Nit: remove white space
> 
> @@ +170,5 @@
> > +          
> 
> Nit: remove white space
> 
> @@ +285,5 @@
> > +          
> 
> Nit: remove white space
> 
> @@ +331,5 @@
> > +          
> 
> Nit: remove white space

I've removed the trailing spaces.  I could also remove the blank lines, but I'm not sure if that's why you meant.

> @@ +723,5 @@
> > +class Ucs4Test : public ::testing::TestWithParam<Ucs4Case> {
> > +};
> 
> As with the ssl_gtests files, can we collect test class definitions at the
> top?
>
> @@ +779,5 @@
> > +INSTANTIATE_TEST_CASE_P(Ucs4TestCases, Ucs4Test,
> > +                        ::testing::ValuesIn(kUcs4Cases));
> 
> And can we group these macro calls at the bottom?

I've shuffled the file to sort the top-level items by type so they match the existing code.  It does look cleaner.

> @@ +732,5 @@
> > +  memset(utf8, 0, sizeof(utf8));
> 
> We can remove the memset if above we do unsigned char utf8[8] = {0};

Thanks; fixed.

> @@ +733,5 @@
> > +  PRBool result;
> > +  const Ucs4Case testCase = GetParam();
> > +
> > +  memset(utf8, 0, sizeof(utf8));
> > +  nc = htonl(testCase.c);
> 
> At the top, we could do:
> 
> TEST_P(Ucs4Test, ToUtf8) {
>   const Ucs4Case testCase = GetParam();
>   PRUint32 nc = htonl(testCase.c);
>   ...

Yes, and: the `PRBool result`s can be declared at first use instead of C-style.  Also, htons() calls are valid in array element initializers, at least in C++.

> @@ +738,5 @@
> > +
> > +  result = sec_port_ucs4_utf8_conversion_function(PR_FALSE,
> > +    (unsigned char *)&nc, sizeof(nc), utf8, sizeof(utf8), &len);
> > +
> > +  ASSERT_EQ(PR_TRUE, result);
> 
> ASSERT_TRUE(), and everywhere else.

No problem.  (Also, why did I do a bunch of them backwards?  Well, it doesn't matter anymore.)

> @@ +741,5 @@
> > +
> > +  ASSERT_EQ(PR_TRUE, result);
> > +  ASSERT_LT(len, sizeof(utf8));
> > +  EXPECT_EQ(std::string(testCase.utf8), std::string((char *)utf8, len));
> > +  EXPECT_EQ('\0', utf8[len]);
> 
> I think we should probably stick to either ASSERT_* or EXPECT_*, they seem
> to have the same implementations, right?

ASSERT ends the current test on failure; EXPECT doesn't.  I used ASSERT for errors where further tests would be meaningless (reported failure, bad lengths, etc.), and EXPECT otherwise.

> @@ +747,5 @@
> > +
> > +TEST_P(Ucs4Test, FromUtf8) {
> > +  PRUint32 nc;
> > +  unsigned int len = 0;
> > +  PRBool result;
> 
> bool result;

Casting from PRBool to bool will break the Windows opt build (warnings-as-errors + MSVC “optimization warning” about int-to-bool casts).  This is also true for explicit static_cast<bool>; it has to marked with an MSVC-specific pragma to disable the warning, which gtest's {ASSERT,EXPECT}_{TRUE,FALSE} do, or else rewritten as an explicit equality comparison.  I'd rather leave these as PRBool, since that's what the code under test returns.

> @@ +975,5 @@
> > +  Utf16BadCase srcBuf;
> > +  unsigned char destBuf[18];
> > +  unsigned int len;
> > +  const Utf16BadCase testCase = GetParam();
> > +  static const size_t maxLen = sizeof(srcBuf.w) / sizeof(srcBuf.w[0]);
> 
> I think we could do:
> 
> static const size_t maxLen = PR_ARRAY_SIZE(srcBuf.w);

Nice; I didn't know about that macro.

> ::: tests/all.sh
> @@ +161,5 @@
> >      NSS_ENABLE_PKIX_VERIFY="1"
> >      export NSS_ENABLE_PKIX_VERIFY
> >  
> >      TESTS="${ALL_TESTS}"
> > +    TESTS_SKIP="cipher dbtests sdr crmf smime merge multinit util_gtests"
> 
> Do we need to add this here? None of the other *_gtests is in here.

I'm pretty sure nssutil isn't affected by whether libpkix is in use?

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Jed Davis [:jld] from comment #7)
> (In reply to Tim Taubert [:ttaubert] from comment #6)
> > ::: external_tests/util_gtest/manifest.mn
> > @@ +1,1 @@
> > > +# -*- makefile -*-
> > 
> > I think this is wrong?
> 
> How so?  That there's an emacs mode line there at all, or the way it's
> formatted, or...?  I can see the point that marking up manifest.mn files
> like that ought to be done across the tree and as a separate bug if it's to
> be done at all.

Just based on the fact that this isn't actually a Makefile. But I don't care too much about emacs anyway.

> > ::: external_tests/util_gtest/util_utf8_unittest.cc
> > @@ +17,5 @@
> > > +struct Ucs4Case {
> > > +  PRUint32 c;
> > 
> > Please s/PRUint32/uint32_t in this file.
> >
> > @@ +22,5 @@
> > > +struct Ucs2Case {
> > > +  PRUint16 c;
> > 
> > uint16_t (and so on... :)
> 
> I'd left them that way to match the NSS interfaces, but these are being
> passed through hton[ls] anyway.  They seem to be the same types on the
> systems I have available, at least.  (I'm willing to assume that uint32_t
> and PRUint32 are bit-representation-equivalent, but something like 32-bit
> int vs. 32-bit long could cause a build failure, so I wanted to check.)

I'm fine with also leaving those untouched.

> > @@ +18,5 @@
> > > +struct Ucs4Case {
> > > +  PRUint32 c;
> > > +  const char *utf8;
> > 
> > uint8_t*
> 
> That breaks everything: unsigned char * can't be initialized with a string
> literal.  (Among other problems.)  I could cast them all, I guess, but... is
> that really an improvement?

Ah, right. Please ignore.

> > Nit: remove white space
> 
> I've removed the trailing spaces.  I could also remove the blank lines, but
> I'm not sure if that's why you meant.

No, just the spaces :) The lines are good as separators.

> > @@ +733,5 @@
> > > +  PRBool result;
> > > +  const Ucs4Case testCase = GetParam();
> > > +
> > > +  memset(utf8, 0, sizeof(utf8));
> > > +  nc = htonl(testCase.c);
> > 
> > At the top, we could do:
> > 
> > TEST_P(Ucs4Test, ToUtf8) {
> >   const Ucs4Case testCase = GetParam();
> >   PRUint32 nc = htonl(testCase.c);
> >   ...
> 
> Yes, and: the `PRBool result`s can be declared at first use instead of
> C-style.  Also, htons() calls are valid in array element initializers, at
> least in C++.

Cool, let's do that.

> > @@ +741,5 @@
> > > +
> > > +  ASSERT_EQ(PR_TRUE, result);
> > > +  ASSERT_LT(len, sizeof(utf8));
> > > +  EXPECT_EQ(std::string(testCase.utf8), std::string((char *)utf8, len));
> > > +  EXPECT_EQ('\0', utf8[len]);
> > 
> > I think we should probably stick to either ASSERT_* or EXPECT_*, they seem
> > to have the same implementations, right?
> 
> ASSERT ends the current test on failure; EXPECT doesn't.  I used ASSERT for
> errors where further tests would be meaningless (reported failure, bad
> lengths, etc.), and EXPECT otherwise.

Interesting, that's good to know.

> > @@ +747,5 @@
> > > +
> > > +TEST_P(Ucs4Test, FromUtf8) {
> > > +  PRUint32 nc;
> > > +  unsigned int len = 0;
> > > +  PRBool result;
> > 
> > bool result;
> 
> Casting from PRBool to bool will break the Windows opt build
> (warnings-as-errors + MSVC “optimization warning” about int-to-bool casts). 
> This is also true for explicit static_cast<bool>; it has to marked with an
> MSVC-specific pragma to disable the warning, which gtest's
> {ASSERT,EXPECT}_{TRUE,FALSE} do, or else rewritten as an explicit equality
> comparison.  I'd rather leave these as PRBool, since that's what the code
> under test returns.

Ok, fair enough.

> > ::: tests/all.sh
> > @@ +161,5 @@
> > >      NSS_ENABLE_PKIX_VERIFY="1"
> > >      export NSS_ENABLE_PKIX_VERIFY
> > >  
> > >      TESTS="${ALL_TESTS}"
> > > +    TESTS_SKIP="cipher dbtests sdr crmf smime merge multinit util_gtests"
> > 
> > Do we need to add this here? None of the other *_gtests is in here.
> 
> I'm pretty sure nssutil isn't affected by whether libpkix is in use?

Ah, that's what this is for :) Thanks for explaining, let's leave it there.

Comment 9

[Jed Davis [:jld] (⏰UTC-6)]

Created attachment 8728637 [details] [diff] [review]
Patch, part 1: add gtests [v2]

Bonus fix: deal with Windows, which doesn't have <netinet/in.h>, by using NSPR's PR_[nh]to[hn][ls] instead.

Comment 10

[Jed Davis [:jld] (⏰UTC-6)]

Comment on attachment 8728637 [details] [diff] [review]
Patch, part 1: add gtests [v2]

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

::: external_tests/util_gtest/util_utf8_unittest.cc
@@ +8,5 @@
> +
> +#include "gtest/gtest.h"
> +#include "prnetdb.h"
> +
> +#include <stdint.h>

Note to self: I should take this back out, now that everything is back to PRUintN types instead of uintN_t.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8728637 [details] [diff] [review]
Patch, part 1: add gtests [v2]

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

Thanks!

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/projects/nss/rev/3360f04a5950
https://hg.mozilla.org/projects/nss/rev/8950d720f796

Comment 13

[Jed Davis [:jld] (⏰UTC-6)]

Created attachment 8730436 [details] [diff] [review]
Patch, part 1.1: Fix build breakage.

This broke the build (https://circleci.com/gh/nss-dev/nss/373); I've reproduced locally (needs GCC 4.6; doesn't happen with 4.8 -Wall -Werror) and this fixes it.  If the broken patch gets backed out before someone can review and land this I can squash them instead.

Comment 14

[Jed Davis [:jld] (⏰UTC-6)]

Created attachment 8730438 [details] [diff] [review]
Patch, part 1.1: Fix build breakage. [v2]

Slight improvement.

Comment 15

[Martin Thomson [:mt:]]

Created attachment 8730508 [details] [diff] [review]
utilfix.patch

Backed out for bustage: https://hg.mozilla.org/projects/nss/rev/0af5db46bd80

Maybe we can get a rollup and we can re-land this.  Also, EXPECT_EQ >> ASSERT_EQ for most of those tests.

Note that the two changes in the fix don't include all the possible signed/unsigned comparisons.  See attached.

Comment 16

[Jed Davis [:jld] (⏰UTC-6)]

Created attachment 8730885 [details] [diff] [review]
Patch, part 1: add gtests [v3]

The other patch fixed the comparisons that GCC 4.6 complained about when I built with it locally, but I agree that it's better to just fix them all.  (This is effectively attachment 8728637 [details] [diff] [review] + attachment 8730508 [details] [diff] [review].)

Comment 17

[Martin Thomson [:mt:]]

Comment on attachment 8730885 [details] [diff] [review]
Patch, part 1: add gtests [v3]

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

::: external_tests/util_gtest/util_utf8_unittest.cc
@@ +287,5 @@
> +
> +  ASSERT_TRUE(result);
> +  ASSERT_LT(len, sizeof(utf8));
> +  EXPECT_EQ(std::string(testCase.utf8), std::string((char *)utf8, len));
> +  EXPECT_EQ('\0', utf8[len]);

Should this be 0U too?

@@ +342,5 @@
> +    &from, sizeof(from), (unsigned char *)&to, sizeof(to), &len);
> +
> +  ASSERT_TRUE(result);
> +  ASSERT_EQ(sizeof(to), len);
> +  ASSERT_EQ(0U, to);

Please use EXPECT_* rather than ASSERT_* unless you need the test to abort when the test fails.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8730885 [details] [diff] [review]
Patch, part 1: add gtests [v3]

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

r=me with mt's comments addressed.

Comment 19

[Jed Davis [:jld] (⏰UTC-6)]

(In reply to Martin Thomson [:mt:] from comment #17)
> ::: external_tests/util_gtest/util_utf8_unittest.cc
> @@ +287,5 @@
> > +
> > +  ASSERT_TRUE(result);
> > +  ASSERT_LT(len, sizeof(utf8));
> > +  EXPECT_EQ(std::string(testCase.utf8), std::string((char *)utf8, len));
> > +  EXPECT_EQ('\0', utf8[len]);
> 
> Should this be 0U too?

Probably?  If I'm following the explanation in K&R, they'll both promote to int, and this compiler seems to think that assuming a signed int can represent all values of uint8_t is a safe assumpiton, but might as well make it unambiguous.

> 
> @@ +342,5 @@
> > +    &from, sizeof(from), (unsigned char *)&to, sizeof(to), &len);
> > +
> > +  ASSERT_TRUE(result);
> > +  ASSERT_EQ(sizeof(to), len);
> > +  ASSERT_EQ(0U, to);
> 
> Please use EXPECT_* rather than ASSERT_* unless you need the test to abort
> when the test fails.

Thanks; I was trying to do this in general, but I missed these.

Comment 20

[Jed Davis [:jld] (⏰UTC-6)]

Created attachment 8731789 [details] [diff] [review]
Patch, part 1: add gtests [v4]

Applied review comments; carrying over r+.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/projects/nss/rev/baff54a27920
https://hg.mozilla.org/projects/nss/rev/662cd5effcb3