Add the first unit test #53

supriyagarg · 2018-02-14T23:21:23Z

This PR sets up the googletest framework, and adds a unit test for format.cc

bmoyles0117

Looks great overall, excited to see tests! One minor comment.

bmoyles0117 · 2018-02-15T15:50:13Z

src/Makefile

-	    $(CPP_NETLIB_DIR) $(YAML_CPP_DIR)
-	git submodule deinit --all
+	$(RM) -r init-submodules build-cpp-netlib build-yaml-cpp
+	git submodule deinit -f $(CPP_NETLIB_DIR) && \


Do these commands need to be chained together? If so, can we indent the second line? Same elsewhere.

I edited the existing Makefile, so have kept the same format.

I have updated the indents in the new Makefile though. PTAL.

bmoyles0117 · 2018-02-16T00:21:23Z

Just providing some context around an offline conversation. Igor confirmed the issues with the Makefile on xenial so he is going to work on fixing up the Makefile issues in a separate pull request, and then we'll rebase your changes so that they can focus on the unit test functionality.

Update the Makefile to load specific submodules.

Also add a Makefile that is easy to extend as we add tests for the remaining code. Only the SRCS needs to be extended in most cases.

bmoyles0117

LGTM

igorpeshansky

A preliminary review.

igorpeshansky · 2018-02-14T23:45:22Z

test/Makefile

+CPPFLAGS+=-isystem $(GTEST_DIR)/include
+CXXFLAGS=-std=c++11 -g -pthread
+
+# Where to find user code.


s/user code/code under test/

igorpeshansky · 2018-02-14T23:48:20Z

src/Makefile

@@ -123,15 +123,14 @@ clean:
 	$(RM) metadatad $(OBJS)

 purge: clean
-	$(RM) -r init-submodules build-cpp-netlib build-yaml-cpp \
-	    $(CPP_NETLIB_DIR) $(YAML_CPP_DIR)
-	git submodule deinit --all


I'm confused. Is this just do we don't touch the googletest submodule during regular builds?

igorpeshansky · 2018-02-14T23:53:44Z

src/Makefile

-	git submodule deinit --all
+	$(RM) -r init-submodules build-cpp-netlib build-yaml-cpp
+	git submodule deinit -f $(CPP_NETLIB_DIR) && \
+	git submodule deinit -f $(YAML_CPP_DIR)


Just list both of them on the same line:

git submodule deinit -f $(CPP_NETLIB_DIR) $(YAML_CPP_DIR)

igorpeshansky · 2018-02-14T23:54:53Z

src/Makefile

-	touch init-submodules
+	git submodule update --init $(CPP_NETLIB_DIR) && \
+	git submodule update --init $(YAML_CPP_DIR)
+	touch ../init-submodules


Careful! init-submodules is an actual file target. You can't just move it to the parent directory.

igorpeshansky · 2018-02-14T23:59:30Z

src/Makefile

-	git submodule init && \
-	git submodule update
-	touch init-submodules
+	git submodule update --init $(CPP_NETLIB_DIR) && \


Same here, you can just put both of them on the same line:

git submodule update --init $(CPP_NETLIB_DIR) $(YAML_CPP_DIR)

igorpeshansky · 2018-02-27T02:49:43Z

test/Makefile

+purge: clean
+	git submodule deinit -f --all
+
+init-submodules:


Unless you want to re-init the submodules all the time, you shouldn't make init-submodules a phony target.

You need to also touch it, like this.

igorpeshansky · 2018-02-27T02:50:33Z

test/Makefile

+	$(CXX) $(CPPFLAGS) -I$(GTEST_DIR) $(CXXFLAGS) -c \
+	    $(GTEST_SOURCEDIR)/gtest_main.cc
+
+$(GTEST_LIBS): build-googletest


Why have a phony target in the middle? This will re-archive every time. Just make $(GTEST_LIBS) depend on $(GTEST_DIR)/Makefile, gtest_main.a and the object files. Or make the target an actual file.

I made the target an actual file - now the only phony targets are clean, purge and all

If that file is never created (and cleaned up), it might as well be a phony target... I was proposing this:

$(GTEST_LIBS): $(GTEST_DIR)/Makefile $(AR) $(ARFLAGS) gtest_main.a gtest-all.o gtest_main.o

igorpeshansky · 2018-02-27T02:57:19Z

test/format_unittest.cc

+
+TEST(SubstituteTest, ValidFormat) {
+  const std::string endpoint_format =
+      "https://stackdriver.googleapis.com/v1beta2/projects/{{project_id}}";


I wonder if we need to keep the original format string here. How about something like:

const std::string format_str("prefix|{{subst}}|postfix"); EXPECT_EQ("prefix|value|postfix", format::Substitute(format_str, {{"subst", "value"}}));

? Ditto for other tests.

Doesn't have to be. But it makes the test more descriptive of the use cases.
Updated.

igorpeshansky · 2018-02-27T02:57:54Z

test/format_unittest.cc

+
+namespace {
+
+TEST(SubstituteTest, ValidFormat) {


How about SingleSubstitution?

Sounds better. Done.

igorpeshansky · 2018-02-27T02:58:07Z

test/format_unittest.cc

+  );
+}
+
+TEST(SubstituteTest, ValidFormatMultipleSubstitution) {


How about just MultipleSubstitutions?

igorpeshansky

Not done with this round, but some early comments.

igorpeshansky · 2018-03-01T21:58:50Z

test/Makefile

+# Where to find user code.
+SRC_DIR=../src
+SRCS=\
+     $(SRC_DIR)/format.cc


No, I meant something like:

../src/%.o: ../src/%.cc cd ../src && $(MAKE) $(subst $@,../src/,)

Alternatively, you could just have:

$(OBJS): ../src/metadatad ../src/metadatad: cd ../src && $(MAKE) all

igorpeshansky · 2018-03-01T22:01:53Z

test/Makefile

+purge: clean
+	git submodule deinit -f --all
+
+init-submodules:


You need to also touch it, like this.

igorpeshansky · 2018-03-01T22:02:45Z

test/Makefile

+
+$(OBJS): build-source
+$(TEST_OBJS): build-test
+unittest: $(GTEST_LIBS) $(OBJS) $(TEST_OBJS)


What I'm saying is: should we name this format-test instead of unittest? Because we'll have tests for other modules.

igorpeshansky · 2018-03-01T22:05:53Z

test/Makefile

+	$(CXX) $(CPPFLAGS) -I$(GTEST_DIR) $(CXXFLAGS) -c \
+	    $(GTEST_SOURCEDIR)/gtest_main.cc
+
+$(GTEST_LIBS): build-googletest


If that file is never created (and cleaned up), it might as well be a phony target... I was proposing this:

$(GTEST_LIBS): $(GTEST_DIR)/Makefile $(AR) $(ARFLAGS) gtest_main.a gtest-all.o gtest_main.o

igorpeshansky · 2018-03-01T22:07:09Z

test/Makefile

+              $(GTEST_DIR)/include/gtest/internal/*.h
+GTEST_SRCS_=$(GTEST_SOURCEDIR)/*.cc $(GTEST_SOURCEDIR)/*.h $(GTEST_HEADERS)
+
+CPP_NETLIB_DIR=$(LIBDIR)/cpp-netlib


Ok, let's add a TODO, please?

igorpeshansky · 2018-03-01T22:13:03Z

test/format_unittest.cc

+}
+
+TEST(SubstituteTest, MultipleSubstitutions) {
+  const std::string format_str("prefix|{{subst}}|{{another_subst}}|postfix");


[Optional] "prefix|{{first}}|middle|{{second}}|suffix".

igorpeshansky · 2018-03-01T22:14:36Z

test/format_unittest.cc

+namespace {
+
+TEST(SubstituteTest, SingleSubstitution) {
+  const std::string format_str("prefix|{{subst}}|postfix");


[Optional] I know I suggested postfix in the first place, but suffix might read slightly better... Up to you.

igorpeshansky · 2018-03-01T22:17:10Z

test/format_unittest.cc

+  EXPECT_EQ(
+      "prefix|value|value2|postfix",
+      format::Substitute(
+          format_str, {{"subst", "value"}, {"another_subst", "value2"}})


[Optional] {{"first", "one"}, {"second", "two"}}.

igorpeshansky · 2018-03-01T22:20:54Z

test/format_unittest.cc

+TEST(SubstituteTest, UnknownParameter) {
+  const std::string format_str("prefix|{{subst}}|postfix");
+  EXPECT_THROW(
+      format::Substitute(format_str, {{"another_subst", "value"}}),


[Optional] {{"unknown", "value"}}.

bmoyles0117

LGTM

igorpeshansky

A couple of stragglers.

igorpeshansky · 2018-03-01T23:57:14Z

test/Makefile

+
+TEST_DIR=.
+TEST_SOURCES=$(wildcard $(TEST_DIR)/*_unittest.cc)
+TEST_OBJS=$(TEST_SOURCES:%.cc=%.o)


I think this needs to be:

TEST_OBJS=$(TEST_SOURCES:$(TEST_DIR)/%.cc=%.o)

Otherwise all TEST_OBJS are going to be in $(TEST_DIR)...

Discussed offline. TEST_DIR is '.', so this works fine.

[Optional] I realized later that $(TEST_DIR) is ., so this is optional, but may be worth doing in case we end up putting tests in a subdirectory, lest we forget to override this (since the test executables do depend on the .o files being in the current directory).

Given that we already have the $(TEST_DIR) variable, makes sense to use it explicitly. Done.

igorpeshansky · 2018-03-02T00:01:36Z

test/Makefile

+
+gtest-all.o: $(GTEST_SOURCEDIR)/gtest-all.cc
+	$(CXX) $(CPPFLAGS) -I$(GTEST_DIR) $(CXXFLAGS) -c -o $@ \
+	    $(GTEST_SOURCEDIR)/gtest-all.cc


This can now be $^, right? And will fit on the previous line.
Ditto for gtest_main.cc

igorpeshansky · 2018-03-02T00:06:48Z

test/Makefile

+
+
+$(SRC_DIR)/%.o: $(SRC_DIR)/%.cc
+	cd $(SRC_DIR) && $(MAKE) $(subst $@,$(SRC_DIR)/,)


I may have gotten this wrong. The following works: $(MAKE) $(@:$(SRC_DIR)/%=%).

igorpeshansky

LGTM
But see some optional comments.

igorpeshansky · 2018-03-02T21:27:13Z

test/Makefile

+
+TEST_DIR=.
+TEST_SOURCES=$(wildcard $(TEST_DIR)/*_unittest.cc)
+TEST_OBJS=$(TEST_SOURCES:%.cc=%.o)


[Optional] I realized later that $(TEST_DIR) is ., so this is optional, but may be worth doing in case we end up putting tests in a subdirectory, lest we forget to override this (since the test executables do depend on the .o files being in the current directory).

igorpeshansky · 2018-03-02T21:28:55Z

test/Makefile

+$(GTEST_SOURCEDIR)/gtest-all.cc $(GTEST_SOURCEDIR)/gtest_main.cc: init-submodules
+
+gtest-all.o: $(GTEST_SOURCEDIR)/gtest-all.cc
+	$(CXX) $(CPPFLAGS) -I$(GTEST_DIR) $(CXXFLAGS) -c -o $@ $^


[Optional] We could make it

$(CXX) -c $(CPPFLAGS) -I$(GTEST_DIR) $(CXXFLAGS) -o $@ $^

to conform to the implicit rule. WDYT? Also below.

Done - moved CXXFLAGS up as well.

igorpeshansky · 2018-03-02T21:33:55Z

test/Makefile

+	$(AR) $(ARFLAGS) $@ $^
+
+format_unittest: $(GTEST_LIB) format_unittest.o $(SRC_DIR)/format.o
+	$(CXX) $(CPPFLAGS) $(CXXFLAGS) $^ -lpthread -o $@


[Optional] We could make it:

$(CXX) $(LDFLAGS) $^ $(LDLIBS) -o $@

and set LDLIBS to -lpthread (again, to ~ conform to the implicit rule).

what about LDFLAGS? set that to CPPFLAGS?

igorpeshansky

Minor.

igorpeshansky · 2018-03-02T21:57:42Z

test/Makefile

+$(GTEST_SOURCEDIR)/gtest-all.cc $(GTEST_SOURCEDIR)/gtest_main.cc: init-submodules
+
+gtest-all.o: $(GTEST_SOURCEDIR)/gtest-all.cc
+	$(CXX) -c $(CPPFLAGS) $(CXXFLAGS) -I$(GTEST_DIR) -o $@ $^


This -I$(GTEST_DIR) is part of $(CPPFLAGS), so let's put it right after (before $(CXXFLAGS)).

igorpeshansky · 2018-03-02T21:58:51Z

test/Makefile

+
+CPPFLAGS+=-isystem $(GTEST_DIR)/include
+CXXFLAGS=-std=c++11 -g -pthread
+LDFLAGS=$(CPPFLAGS)


No need for this -- the linker ignores the -isystem arguments. Just let $(LDFLAGS) pick up the default (i.e., don't set it).

igorpeshansky · 2018-03-02T22:00:38Z

test/Makefile

+CPPFLAGS+=-isystem $(GTEST_DIR)/include
+CXXFLAGS=-std=c++11 -g -pthread
+LDFLAGS=$(CPPFLAGS)
+LDLIBS=-lpthread


[Optional] This may actually be implied by -pthread, but it's harmless to keep as-is.

This is required.

This may not have been required had we set LDFLAGS=$(CXXFLAGS), but it's not a big deal either way.

igorpeshansky

LGTM

supriyagarg requested review from bmoyles0117 and igorpeshansky February 14, 2018 23:21

bmoyles0117 approved these changes Feb 15, 2018

View reviewed changes

bmoyles0117 reviewed Feb 15, 2018

View reviewed changes

igorpeshansky mentioned this pull request Feb 16, 2018

Explicitly list the submodules during init and deinit. #54

Merged

supriyagarg force-pushed the master branch from cf1bd6e to 4c5d652 Compare February 16, 2018 17:45

supriyagarg added 3 commits February 16, 2018 12:51

Add googletest as a submodule.

4be8913

Update the Makefile to load specific submodules.

Add a unitttest for format.cc

1a2201f

Also add a Makefile that is easy to extend as we add tests for the remaining code. Only the SRCS needs to be extended in most cases.

Fix the indentation and add .PHONY in test/Makefile

1cdba2b

supriyagarg force-pushed the master branch from 4c5d652 to 1cdba2b Compare February 16, 2018 17:58

bmoyles0117 approved these changes Feb 16, 2018

View reviewed changes

igorpeshansky suggested changes Feb 27, 2018

View reviewed changes

igorpeshansky force-pushed the master branch from 70449da to 5aa2078 Compare February 28, 2018 00:00

Address comments

bbc20ff

igorpeshansky suggested changes Mar 1, 2018

View reviewed changes

Address comments

626e1d7

bmoyles0117 approved these changes Mar 2, 2018

View reviewed changes

igorpeshansky suggested changes Mar 2, 2018

View reviewed changes

Address comments

10c0a4a

igorpeshansky approved these changes Mar 2, 2018

View reviewed changes

Address comments

d8d0def

igorpeshansky suggested changes Mar 2, 2018

View reviewed changes

reorder flags and use implicit rules in Makefile

ae23d77

igorpeshansky approved these changes Mar 2, 2018

View reviewed changes

igorpeshansky merged commit adde53f into Stackdriver:master Mar 2, 2018



		$(SRC_DIR)/%.o: $(SRC_DIR)/%.cc
		cd $(SRC_DIR) && $(MAKE) $(subst $@,$(SRC_DIR)/,)


		namespace {

		TEST(SubstituteTest, ValidFormat) {

Add the first unit test #53

Add the first unit test #53

Conversation

supriyagarg commented Feb 14, 2018

bmoyles0117 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmoyles0117 commented Feb 16, 2018

bmoyles0117 left a comment

Choose a reason for hiding this comment

igorpeshansky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igorpeshansky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmoyles0117 left a comment

Choose a reason for hiding this comment

igorpeshansky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igorpeshansky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igorpeshansky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igorpeshansky left a comment

Choose a reason for hiding this comment