Implement an integration test for Table::Apply() #74

coryan · 2017-12-06T02:36:07Z

This fixes #37. It implements an integration test for Table::Apply() against the Cloud Bigtable Emulator.

I think both @sduskis and @mbrukman will have opinions on the need for such integration tests. I want to hear from them if this is good enough. @garye and @DanielMahu please check the code too!

This is part of the fixes for googleapis#37. The script starts up the emulator and runs the integration test against it.

Also run the integration tests as part of the CI builds.

mbrukman

Some thoughts for your consideration.

mbrukman · 2017-12-06T03:55:26Z

ci/Dockerfile.fedora

    wget \
    zlib-devel

+# Install the bigtable emulator and the bigtable command-line client.  They


s/bigtable/Bigtable/g

mbrukman · 2017-12-06T03:55:42Z

ci/Dockerfile.ubuntu

    g++-4.8 \
  || /bin/true

+# Install the bigtable emulator and the bigtable command-line client.  They


s/bigtable/Bigtable/

mbrukman · 2017-12-06T03:58:43Z

bigtable/tests/apply_test.cc

+
+int main() try {
+  bigtable::Client client("emulated", "emulated");
+  std::unique_ptr<bigtable::Table> table = client.Open("test-table");


Do we have the Admin API available yet? If so, it should create and delete the table itself. Same for the column family.

Otherwise, it's a bunch of magic strings here: both table and column family are set up via script, and must match here, but it's not stated anywhere either in the shell script or this C++ test.

Some options:

document the implicit dependency (easy and cheesy)

set env vars in the script, read those env vars in C++ so that it's clear it's an external input (preferred, unless option 3 is coming soon)

once Admin API is available, just create/delete table + column family in the script (defer)

Thoughts?

Nope, there is no Admin API, nor is the Admin API part of the MVP. I have created #79 for this. I am inclined to do (2) because it will make this program reusable for testing against production too. I used command-line args for the first cut.

mbrukman · 2017-12-06T03:59:18Z

bigtable/tests/apply_test.cc

+  mutation.emplace_back(bigtable::SetCell("fam", "col0", 0, "value-1-0"));
+  mutation.emplace_back(bigtable::SetCell("fam", "col1", 0, "value-1-1"));
+  table->Apply(std::move(mutation));
+  std::cout << "row-key-1 mutated successfully\n";


Should this also do a read after each write to ensure it actually wrote what it thinks it did?

It should, but we do not have any APIs to read yet. #32 #29

mbrukman · 2017-12-06T03:59:28Z

bigtable/tests/run_integration_tests.sh

+set -eu
+
+function kill_emulator {
+    kill ${EMULATOR_PID}


2- or 4-space indent for shell scripts?

mbrukman · 2017-12-06T04:01:32Z

ci/Dockerfile.ubuntu

+ARG GOPATH=/var/tmp/build/cbt
+WORKDIR ${GOPATH}
+RUN if grep -q 14.04 /etc/lsb-release; then \
+      echo "Skip cloud bigtable client install, go version too old in Ubuntu 14.04."; \


mbrukman · 2017-12-06T04:01:43Z

ci/Dockerfile.ubuntu

+ARG GOPATH=/var/tmp/build/cbt
+WORKDIR ${GOPATH}
+RUN if grep -q 14.04 /etc/lsb-release; then \
+      echo "Skip cloud bigtable client install, go version too old in Ubuntu 14.04."; \


s/Skip/Skipping/ ?

mbrukman · 2017-12-06T04:01:48Z

ci/Dockerfile.ubuntu

+
+WORKDIR /var/tmp/build/gccpp/build-output/bigtable
+RUN if grep -q 14.04 /etc/lsb-release; then \
+       echo "Skip integration tests, go version too old in Ubuntu 14.04."; \


s/Skip/Skipping/ ?

mbrukman · 2017-12-06T04:02:25Z

bigtable/tests/run_integration_tests.sh

+"${GOPATH}/bin/emulator" >emulator.log 2>&1 </dev/null &
+EMULATOR_PID=$!
+if [ $? -ne 0 ]; then
+    echo "emulator failed, aborting"


How about:

echo "Cloud Bigtable emulator failed; aborting test." >&2

?

mbrukman · 2017-12-06T04:05:12Z

bigtable/tests/run_integration_tests.sh

+
+trap kill_emulator EXIT
+
+export BIGTABLE_EMULATOR_HOST=localhost:9000


Is 9000 the default port on which the emulator listens on? Is this running inside a container so we're guaranteed that 9000 is not taken already? Is it worthwhile to document where this magic number comes from? Is it worthwhile to choose a specific port explicitly and use a var so the connection is clear?

mbrukman · 2017-12-07T03:23:20Z

bigtable/tests/run_integration_tests.sh


 echo "Launching Cloud Bigtable emulator in the background"
-"${GOPATH}/bin/emulator" >emulator.log 2>&1 </dev/null &
+# The tests typically run on a docker container, where the ports are largely


s/on a docker/in a Docker/ ?

mbrukman · 2017-12-07T03:23:45Z

bigtable/tests/run_integration_tests.sh

 echo "Launching Cloud Bigtable emulator in the background"
-"${GOPATH}/bin/emulator" >emulator.log 2>&1 </dev/null &
+# The tests typically run on a docker container, where the ports are largely
+# free, when using in manual tests you can set EMULATOR_PORT.


s/free,/free;/

s/manual tests/manual tests,/

Done, thanks!

mbrukman · 2017-12-07T03:24:57Z

ci/Dockerfile.ubuntu

 WORKDIR ${GOPATH}
 RUN if grep -q 14.04 /etc/lsb-release; then \
-      echo "Skip cloud bigtable client install, go version too old in Ubuntu 14.04."; \
+      echo "Skipping cloud bigtable client install, Go version too old in Ubuntu 14.04."; \


s/cloud bigtable client/cbt/ ?

or if you want to be more descriptive, "cbt CLI"?

@mbrukman

Thanks to @mbrukman.

coryan · 2017-12-07T03:54:18Z

PTAL.

mbrukman

LGTM

mbrukman · 2017-12-07T04:15:30Z

bigtable/tests/run_integration_tests.sh

+
+echo "Creating test-table in the emulator."
+"${GOPATH}/bin/cbt" $CBT_ARGS createtable test-table
+echo "Creating family in test-table."


s/family/column family/

Argh, I missed that. Will fix.

Fixed in #81

Leftover review comment from PR #74.

coryan added 3 commits December 5, 2017 21:29

Create very simple integration test.

84d0e68

This is part of the fixes for googleapis#37. The script starts up the emulator and runs the integration test against it.

Add driver script to run integration tests.

c1712ee

Also run the integration tests as part of the CI builds.

Fix formatting with clang-format.

546901d

coryan requested review from DanielMahu, garye, mbrukman and sduskis December 6, 2017 02:36

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 6, 2017

mbrukman suggested changes Dec 6, 2017

View reviewed changes

coryan added 2 commits December 6, 2017 09:15

Address comments from review.

48ce471

How about I get the code to compile too?

ee73b46

mbrukman suggested changes Dec 7, 2017

View reviewed changes

Address additional comments in review.

7bdec09

Thanks to @mbrukman.

mbrukman approved these changes Dec 7, 2017

View reviewed changes

coryan merged commit 67fc1ff into googleapis:master Dec 7, 2017

coryan deleted the fix-issue-37-integration-bigtable-emulator-develop branch December 7, 2017 04:21

mbrukman pushed a commit that referenced this pull request Dec 9, 2017

Fix test message [skip ci] (#81)

c00f0bc

Leftover review comment from PR #74.


		trap kill_emulator EXIT

		export BIGTABLE_EMULATOR_HOST=localhost:9000

Implement an integration test for Table::Apply() #74

Implement an integration test for Table::Apply() #74

Uh oh!

Conversation

coryan commented Dec 6, 2017

Uh oh!

mbrukman 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!

coryan commented Dec 7, 2017

Uh oh!

mbrukman 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants