Base64 unit tests by ACEmilG · Pull Request #71 · Stackdriver/metadata-agent

ACEmilG · 2018-03-09T23:30:19Z

Add testing for base64 encoding. Remove base64 decoding from header file to reflect its commented out implementation.

igorpeshansky

Thanks. Please try to apply each comment everywhere it makes sense.

igorpeshansky · 2018-03-10T02:08:30Z

 std::string Encode(const std::string &source);

-std::string Decode(const std::string &source);
+//std::string Decode(const std::string &source);


Why did you decide to comment this out?

The definition is commented out, so I thought I would reflect that in the header file. It seemed wrong to have a declaration with no definition anywhere.

I don't feel tied to this - happy to change it back.

Let's uncomment this. To be honest, I've also uncommented the implementation and tested it in my private clone, and it works fine, so we might as well make it available, even if we don't yet use it.
Want me to push the commit that does that?

igorpeshansky · 2018-03-10T02:08:53Z

 TEST_OBJS=$(TEST_SOURCES:$(TEST_DIR)/%.cc=%.o)
 TESTS=\
-      format_unittest
+      format_unittest\


Let's add a space before the \.

igorpeshansky · 2018-03-10T02:11:58Z

+namespace {
+
+TEST(EncodeTest, EmptyEncode) {
+  const std::string empty_str("");


May not be worth naming the constants... How about just inlining them into the EXPECT_EQ? I.e.,

TEST (EncodeTest, EmptyEncode) { EXPECT_EQ("", base64::Encode("")); }

?

Agreed - done.

igorpeshansky · 2018-03-10T02:20:57Z

+  );
+}
+
+TEST(EncodeTest, NoPaddingEncode) {


Do you want to add a comment explaining the "no padding" bit? E.g., // 5 bytes should produce 7 characters.

I've been trying to come up with a good name for the "padding" tests... The best I got so far is "phantom" (as in "phantom characters at the end"). So this test would have been named "OnePhantom", and the double padding one below would become "TwoPhantoms". WDYT? Happy to hear suggestions for better names. Ultimately, I think we ought to explain what we're testing in a comment if the test name isn't sufficiently expressive.
Your comment explains "no padding", but not the difference between the single and double pad characters.

Phantom sounds good to me. Hopefully the new comment is more clear.

igorpeshansky · 2018-03-10T02:27:50Z

+  );
+}
+
+TEST(EncodeTest, NoDoublePaddingEncode) {


Why "double padding"? Can you please elaborate?

Sure: Typically this would have "==" as padding, hence the "double" as opposed to the above which would have "=" (single). I guess its kind of a trivial case, happy to remove it.

I think it's an important corner case and needs to be tested, but see my other note about test naming.

Addressed above.

igorpeshansky · 2018-03-12T14:29:22Z

 std::string Encode(const std::string &source);

-std::string Decode(const std::string &source);
+//std::string Decode(const std::string &source);


Let's uncomment this. To be honest, I've also uncommented the implementation and tested it in my private clone, and it works fine, so we might as well make it available, even if we don't yet use it.
Want me to push the commit that does that?

igorpeshansky · 2018-03-12T14:30:29Z

+namespace {
+
+TEST(EncodeTest, EmptyEncode) {
+  EXPECT_EQ(


Let's save some vertical space and put this whole EXPECT_EQ on a single line. Ditto below.

Agreed, fixed.

igorpeshansky · 2018-03-12T14:31:18Z

+  );
+}
+
+//Base64 encodings typically pad messages to ensure output length % 4 == 0, our


Nit: missing space after comment marker.

igorpeshansky · 2018-03-12T14:31:37Z

+}
+
+//Base64 encodings typically pad messages to ensure output length % 4 == 0, our
+//implementation does not


Nit: missing trailing ".".

igorpeshansky · 2018-03-12T15:38:23Z

+  );
+}
+
+TEST(EncodeTest, NoPaddingEncode) {


I've been trying to come up with a good name for the "padding" tests... The best I got so far is "phantom" (as in "phantom characters at the end"). So this test would have been named "OnePhantom", and the double padding one below would become "TwoPhantoms". WDYT? Happy to hear suggestions for better names. Ultimately, I think we ought to explain what we're testing in a comment if the test name isn't sufficiently expressive.
Your comment explains "no padding", but not the difference between the single and double pad characters.

igorpeshansky · 2018-03-12T15:38:39Z

+  );
+}
+
+TEST(EncodeTest, NoDoublePaddingEncode) {


I think it's an important corner case and needs to be tested, but see my other note about test naming.

…code.

igorpeshansky · 2018-03-12T18:48:00Z

+}
+
+// Base64 encoders typically pad messages to ensure output length % 4 == 0. To
+// acheive this, encoders will pad messages with either one or two "=".  Our


Let's stick to one space after "." (throughout).

igorpeshansky

LGTM

supriyagarg

A very minor comment., otherwise LGTM.

supriyagarg · 2018-03-12T21:38:19Z

+}
+
+// Base64 encoders typically pad messages to ensure output length % 4 == 0. To
+// acheive this, encoders will pad messages with either one or two "=". Our


acheive -> achieve

igorpeshansky

LGTM again.

Also fix bug in alternate base64::Encode implementation and enable base64::Decode.

ACEmilG added 2 commits March 9, 2018 17:00

Initial commit adding unittests for base64

d11f41b

Comment decode out of header file for clarity

a671f16

igorpeshansky suggested changes Mar 10, 2018

View reviewed changes

Update base64 tests and Makefile based on PR 71

9d8bb62

igorpeshansky suggested changes Mar 12, 2018

View reviewed changes

igorpeshansky and others added 4 commits March 12, 2018 12:49

Fix bug in alternate base64::Encode implementation; enable base64::De…

bda6995

…code.

Add base64 round-trip tests.

58b786f

Address further comments on PR 71

8217506

Added further testing for base64::Decode

e50cd9e

igorpeshansky suggested changes Mar 12, 2018

View reviewed changes

One space after periods. Removed whitespace to reflect surrounding style

2e04d0e

igorpeshansky approved these changes Mar 12, 2018

View reviewed changes

igorpeshansky requested a review from supriyagarg March 12, 2018 22:03

supriyagarg reviewed Mar 12, 2018

View reviewed changes

supriyagarg approved these changes Mar 12, 2018

View reviewed changes

Spelling error

893b6f8

igorpeshansky approved these changes Mar 13, 2018

View reviewed changes

igorpeshansky merged commit 39932b0 into master Mar 13, 2018

igorpeshansky deleted the ACEmilG-base64-unit-tests branch March 13, 2018 12:52

igorpeshansky pushed a commit that referenced this pull request Mar 13, 2018

Base64 unit tests (#71)

1aae818

Also fix bug in alternate base64::Encode implementation and enable base64::Decode.

Conversation

ACEmilG commented Mar 9, 2018

Uh oh!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

supriyagarg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants