Added lots of decoder tests and fixed lots of bugs.

3 files changed, 128 insertions, 33 deletions

diff --git a/tests/json/test_json.cc b/tests/json/test_json.cc
index c483cf0..b4346c5 100644
--- a/tests/json/test_json.cc
+++ b/tests/json/test_json.cc
@@ -295,17 +295,15 @@ void test_json_roundtrip_message(const char* json_src,
   upb::json::Parser* parser =
       upb::json::Parser::Create(env.env(), printer->input());
   env.ResetBytesSink(parser->input());
-  env.Reset(json_src, strlen(json_src), false);
+  env.Reset(json_src, strlen(json_src), false, false);
 
   bool ok = env.Start() &&
             env.ParseBuffer(seam) &&
             env.ParseBuffer(-1) &&
             env.End();
 
-  if (!ok) {
-    fprintf(stderr, "upb parse error: %s\n", env.status().error_message());
-  }
   ASSERT(ok);
+  ASSERT(env.CheckConsistency());
 
   if (memcmp(json_expected,
              data_sink.Data().data(),
diff --git a/tests/pb/test_decoder.cc b/tests/pb/test_decoder.cc
index fd9d9ae..94293cb 100644
--- a/tests/pb/test_decoder.cc
+++ b/tests/pb/test_decoder.cc
@@ -32,6 +32,7 @@
 #include <stdint.h>
 #include <stdlib.h>
 #include <string.h>
+#include <sstream>
 
 #include "tests/test_util.h"
 #include "tests/upb_test.h"
@@ -176,6 +177,13 @@ string cat(const string& a, const string& b,
   return ret;
 }
 
+template <typename T>
+string num2string(T num) {
+  std::ostringstream ss;
+  ss << num;
+  return ss.str();
+}
+
 string varint(uint64_t x) {
   char buf[UPB_PB_VARINT_MAX_LEN];
   size_t len = upb_vencode64(x, buf);
@@ -202,6 +210,11 @@ string submsg(uint32_t fn, const string& buf) {
   return cat( tag(fn, UPB_WIRE_TYPE_DELIMITED), delim(buf) );
 }
 
+string group(uint32_t fn, const string& buf) {
+  return cat(tag(fn, UPB_WIRE_TYPE_START_GROUP), buf,
+             tag(fn, UPB_WIRE_TYPE_END_GROUP));
+}
+
 // Like delim()/submsg(), but intentionally encodes an incorrect length.
 // These help test when a delimited boundary doesn't land in the right place.
 string badlen_delim(int err, const string& buf) {
@@ -558,9 +571,9 @@ uint32_t Hash(const string& proto, const string* expected_output, size_t seam1,
 
 void CheckBytesParsed(const upb::pb::Decoder& decoder, size_t ofs) {
   // We could have parsed as many as 10 bytes fewer than what the decoder
-  // previously accepted, since we can buffer up to 10 partial bytes internally
+  // previously accepted, since we can buffer up to 12 partial bytes internally
   // before accumulating an entire value.
-  const int MAX_BUFFERED = 10;
+  const int MAX_BUFFERED = 14;
 
   // We can't have parsed more data than the decoder callback is telling us it
   // parsed.
@@ -582,7 +595,7 @@ static bool parse(VerboseParserEnvironment* env,
 void do_run_decoder(VerboseParserEnvironment* env, upb::pb::Decoder* decoder,
                     const string& proto, const string* expected_output,
                     size_t i, size_t j, bool may_skip) {
-  env->Reset(proto.c_str(), proto.size(), may_skip);
+  env->Reset(proto.c_str(), proto.size(), may_skip, expected_output == NULL);
   decoder->Reset();
 
   testhash = Hash(proto, expected_output, i, j, may_skip);
@@ -598,7 +611,14 @@ void do_run_decoder(VerboseParserEnvironment* env, upb::pb::Decoder* decoder,
       PrintBinary(proto);
       fprintf(stderr, "\n");
       if (expected_output) {
-        fprintf(stderr, "Expected output: %s\n", expected_output->c_str());
+        if (test_mode == ALL_HANDLERS) {
+          fprintf(stderr, "Expected output: %s\n", expected_output->c_str());
+        } else if (test_mode == NO_HANDLERS) {
+          fprintf(stderr,
+                  "No handlers are registered, BUT if they were "
+                  "the expected output would be: %s\n",
+                  expected_output->c_str());
+        }
       } else {
         fprintf(stderr, "Expected to FAIL\n");
       }
@@ -610,7 +630,7 @@ void do_run_decoder(VerboseParserEnvironment* env, upb::pb::Decoder* decoder,
               parse(env, *decoder, -1) &&
               env->End();
 
-    ASSERT(ok == env->status().ok());
+    ASSERT(env->CheckConsistency());
 
     if (test_mode == ALL_HANDLERS) {
       if (expected_output) {
@@ -618,18 +638,12 @@ void do_run_decoder(VerboseParserEnvironment* env, upb::pb::Decoder* decoder,
           fprintf(stderr, "Text mismatch: '%s' vs '%s'\n",
                   output.c_str(), expected_output->c_str());
         }
-        if (!ok) {
-          fprintf(stderr, "Failed: %s\n", env->status().error_message());
-        }
         ASSERT(ok);
         ASSERT(output == *expected_output);
       } else {
         if (ok) {
           fprintf(stderr, "Didn't expect ok result, but got output: '%s'\n",
                   output.c_str());
-        } else if (filter_hash) {
-          fprintf(stderr, "Failed as we expected, with message: %s\n",
-                  env->status().error_message());
         }
         ASSERT(!ok);
       }
@@ -657,6 +671,25 @@ void run_decoder(const string& proto, const string* expected_output) {
 const static string thirty_byte_nop = cat(
     tag(NOP_FIELD, UPB_WIRE_TYPE_DELIMITED), delim(string(30, 'X')) );
 
+// Indents and wraps text as if it were a submessage with this field number
+string wrap_text(int32_t fn, const string& text) {
+  string wrapped_text = text;
+  size_t pos = 0;
+  string replace_with = "\n  ";
+  while ((pos = wrapped_text.find("\n", pos)) != string::npos &&
+         pos != wrapped_text.size() - 1) {
+    wrapped_text.replace(pos, 1, replace_with);
+    pos += replace_with.size();
+  }
+  wrapped_text = cat(
+      LINE("<"),
+      num2string(fn), LINE(":{")
+      "  ", wrapped_text,
+      LINE("}")
+      LINE(">"));
+  return wrapped_text;
+}
+
 void assert_successful_parse(const string& proto,
                              const char *expected_fmt, ...) {
   string expected_text;
@@ -668,16 +701,27 @@ void assert_successful_parse(const string& proto,
   // repeat once with no-op padding data at the end of buffer.
   run_decoder(proto, &expected_text);
   run_decoder(cat( proto, thirty_byte_nop ), &expected_text);
+
+  // Test that this also works when wrapped in a submessage or group.
+  // Indent the expected text one level and wrap it.
+  string wrapped_text1 = wrap_text(UPB_DESCRIPTOR_TYPE_MESSAGE, expected_text);
+  string wrapped_text2 = wrap_text(UPB_DESCRIPTOR_TYPE_GROUP, expected_text);
+
+  run_decoder(submsg(UPB_DESCRIPTOR_TYPE_MESSAGE, proto), &wrapped_text1);
+  run_decoder(group(UPB_DESCRIPTOR_TYPE_GROUP, proto), &wrapped_text2);
 }
 
 void assert_does_not_parse_at_eof(const string& proto) {
   run_decoder(proto, NULL);
 
   // Also test that we fail to parse at end-of-submessage, not just
-  // end-of-message.
-  run_decoder(submsg(UPB_DESCRIPTOR_TYPE_MESSAGE, proto), NULL);
-  run_decoder(cat(submsg(UPB_DESCRIPTOR_TYPE_MESSAGE, proto), thirty_byte_nop),
-              NULL);
+  // end-of-message.  But skip this if we have no handlers, because in that
+  // case we won't descend into the submessage.
+  if (test_mode != NO_HANDLERS) {
+    run_decoder(submsg(UPB_DESCRIPTOR_TYPE_MESSAGE, proto), NULL);
+    run_decoder(cat(submsg(UPB_DESCRIPTOR_TYPE_MESSAGE, proto),
+                    thirty_byte_nop), NULL);
+  }
 }
 
 void assert_does_not_parse(const string& proto) {
@@ -905,11 +949,13 @@ void test_invalid() {
            submsg(UPB_DESCRIPTOR_TYPE_MESSAGE, string("     "))));
 
   // Test exceeding the resource limit of stack depth.
-  string buf;
-  for (int i = 0; i <= MAX_NESTING; i++) {
-    buf.assign(submsg(UPB_DESCRIPTOR_TYPE_MESSAGE, buf));
+  if (test_mode != NO_HANDLERS) {
+    string buf;
+    for (int i = 0; i <= MAX_NESTING; i++) {
+      buf.assign(submsg(UPB_DESCRIPTOR_TYPE_MESSAGE, buf));
+    }
+    assert_does_not_parse(buf);
   }
-  assert_does_not_parse(buf);
 }
 
 void test_valid() {
@@ -987,6 +1033,15 @@ void test_valid() {
 
   // Unknown field inside a known submessage.
   assert_successful_parse(
+      submsg(UPB_DESCRIPTOR_TYPE_MESSAGE, submsg(12345, string("   "))),
+      LINE("<")
+      LINE("%u:{")
+      LINE("  <")
+      LINE("  >")
+      LINE("}")
+      LINE(">"), UPB_DESCRIPTOR_TYPE_MESSAGE);
+
+  assert_successful_parse(
       cat (submsg(UPB_DESCRIPTOR_TYPE_MESSAGE, submsg(12345, string("   "))),
            tag(UPB_DESCRIPTOR_TYPE_INT32, UPB_WIRE_TYPE_VARINT),
            varint(5)),
@@ -1162,7 +1217,9 @@ void test_valid() {
     indentbuf(&textbuf, total - i - 1);
     textbuf.append(">\n");
   }
-  assert_successful_parse(buf, "%s", textbuf.c_str());
+  // Have to use run_decoder directly, because we are at max nesting and can't
+  // afford the extra nesting that assert_successful_parse() will do.
+  run_decoder(buf, &textbuf);
 }
 
 upb::reffed_ptr<const upb::pb::DecoderMethod> NewMethod(
@@ -1207,10 +1264,11 @@ upb::reffed_ptr<const upb::pb::DecoderMethod> method =
     upb::Sink sink(method->dest_handlers(), &closures[0]);
     upb::pb::Decoder* decoder = CreateDecoder(env.env(), method.get(), &sink);
     env.ResetBytesSink(decoder->input());
-    env.Reset(testdata[i].data, testdata[i].length, true);
+    env.Reset(testdata[i].data, testdata[i].length, true, false);
     ASSERT(env.Start());
     ASSERT(env.ParseBuffer(-1));
     ASSERT(env.End());
+    ASSERT(env.CheckConsistency());
   }
 }
 
diff --git a/tests/test_util.h b/tests/test_util.h
index f3d5d5a..caf9ee8 100644
--- a/tests/test_util.h
+++ b/tests/test_util.h
@@ -29,16 +29,35 @@ class VerboseParserEnvironment {
  public:
   // Pass verbose=true to print detailed diagnostics to stderr.
   VerboseParserEnvironment(bool verbose) : verbose_(verbose) {
-    env_.ReportErrorsTo(&status_);
+    env_.SetErrorFunction(&VerboseParserEnvironment::OnError, this);
   }
 
-  void Reset(const char *buf, size_t len, bool may_skip) {
+  static bool OnError(void *ud, const upb::Status* status) {
+    VerboseParserEnvironment* env = static_cast<VerboseParserEnvironment*>(ud);
+
+    env->saw_error_ = true;
+
+    if (env->expect_error_ && env->verbose_) {
+      fprintf(stderr, "Encountered error, as expected: ");
+    } else if (!env->expect_error_) {
+      fprintf(stderr, "Encountered unexpected error: ");
+    } else {
+      return false;
+    }
+
+    fprintf(stderr, "%s\n", status->error_message());
+    return false;
+  }
+
+  void Reset(const char *buf, size_t len, bool may_skip, bool expect_error) {
     buf_ = buf;
     len_ = len;
     ofs_ = 0;
+    expect_error_ = expect_error;
+    saw_error_ = false;
+    end_ok_set_ = false;
     skip_until_ = may_skip ? 0 : -1;
     skipped_with_null_ = false;
-    status_.Clear();
   }
 
   // The user should call a series of:
@@ -63,7 +82,26 @@ class VerboseParserEnvironment {
     if (verbose_) {
       fprintf(stderr, "Calling end()\n");
     }
-    return sink_->End();
+    end_ok_ = sink_->End();
+    end_ok_set_ = true;
+
+    return end_ok_;
+  }
+
+  bool CheckConsistency() {
+    /* If we called end (which we should only do when previous bytes are fully
+     * accepted), then end() should return true iff there were no errors. */
+    if (end_ok_set_ && end_ok_ != !saw_error_) {
+      fprintf(stderr, "End() status and saw_error didn't match.\n");
+      return false;
+    }
+
+    if (expect_error_ && !saw_error_) {
+      fprintf(stderr, "Expected error but saw none.\n");
+      return false;
+    }
+
+    return true;
   }
 
   bool ParseBuffer(int bytes) {
@@ -117,7 +155,7 @@ class VerboseParserEnvironment {
       }
     }
 
-    if (!status_.ok())
+    if (saw_error_)
       return false;
 
     if (parsed > bytes && skip_until_ >= 0) {
@@ -133,8 +171,6 @@ class VerboseParserEnvironment {
     sink_ = sink;
   }
 
-  const upb::Status& status() { return status_; }
-
   size_t ofs() { return ofs_; }
   upb::Environment* env() { return &env_; }
 
@@ -142,13 +178,16 @@ class VerboseParserEnvironment {
 
  private:
   upb::Environment env_;
-  upb::Status status_;
   upb::BytesSink* sink_;
   const char* buf_;
   size_t len_;
   bool verbose_;
   size_t ofs_;
   void *subc_;
+  bool expect_error_;
+  bool saw_error_;
+  bool end_ok_;
+  bool end_ok_set_;
 
   // When our parse call returns a value greater than the number of bytes
   // we passed in, the decoder is indicating to us that the next N bytes