test(storage): add emulator unit tests and cleanup

[vnghia]

Since Bazel does not fully support pytest, I converted all those tests to unittest.

I have some questions

• Should we convert all the code that are using testbench to emulator ?
• When will we remove the testbench directory ?

Part of #4751

We finally reach a milestone in Complete GCS+gRPC plugin. From now, we could implementing GCS+gRPC without worrying about testing 🚀

---

This change is 

[coryan]

Since Bazel does not fully support pytest, I converted all those tests to unittest.

Thanks.

I have some questions

Should we convert all the code that are using testbench to emulator ?

Seems to me all the google-cloud-cpp integration tests are already doing so. What else did you have in mind?

When will we remove the testbench directory ?

I think we should open a bug to track these things, but I believe we need to move and/or create a new README.md file like then one in the testbench/ directory.

Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @vnvo2409)

---

google/cloud/storage/emulator/BUILD, line 15 at r1 (raw file):

# limitations under the License.

py_test(

You need to add some boilerplate I think:

package(default_visibility = ["//visibility:public"])

licenses(["notice"])  # Apache 2.0

[vnghia]

The CI threw /usr/bin/env: 'python': No such file or directory. Do you have any idea why ? @coryan

I think it's because we have python3 in PATH but not python ? what do you think about symbolic link
ln -s /usr/bin/python3 /usr/bin/python

https://source.cloud.google.com/results/invocations/48148a79-4487-4d4e-90d0-30b8e3a1c1be/targets/cloud-cpp%2Fgithub%2Fgoogle-cloud-cpp%2Fmaster%2Fdocker%2Fintegration-presubmit/tests

---

Windows failed because of another reason: Those tests do not support to run on Windows. I have no idea why it happened 😕
https://source.cloud.google.com/results/invocations/5da479e1-b692-4f60-b7ad-96eb0e58111a/targets/cloud-devrel%2Fclient-libraries%2Fcpp%2Fgoogle-cloud-cpp%2Fwindows%2Fbazel-presubmit/log

[coryan]

Searching around I found this:

bazelbuild/bazel#8665

We do not install a plain "python" program in the docker images for these builds:

./ci/kokoro/docker/build.sh asan bash
bash-5.0$ python 
bash: python: command not found
bash-5.0$ python3 --version
Python 3.9.0

There is already an open bug for Bazel (bazelbuild/bazel#8446), but I think we will need to workaround it by installing a plain python program.

[ghost]

FYI Ubuntu only ships /usr/bin/python3 but not /usr/bin/python. On Fedora however, /usr/bin/python is symlinked to /usr/bin/python3. Focal has this package, but I'm not sure how safe the symlink would be on Bionic. WDYT?

[coryan]

This was Fedora:33, and I am not seeing the /usr/bin/python link:

docker run --rm -it fedora:33 bash
# dnf makecache && dnf install python3
# ls -l /usr/bin/py*
lrwxrwxrwx 1 root root     8 Oct  6 14:19 /usr/bin/pydoc -> ./pydoc3
lrwxrwxrwx 1 root root     8 Oct  6 14:19 /usr/bin/pydoc3 -> pydoc3.9
-rwxr-xr-x 1 root root    78 Oct  6 14:19 /usr/bin/pydoc3.9
lrwxrwxrwx 1 root root     9 Oct  6 14:19 /usr/bin/python3 -> python3.9
-rwxr-xr-x 1 root root 15536 Oct  6 14:20 /usr/bin/python3.9

Seems like we need to install python (aka python-unversioned-command) for Fedora:33:

# dnf makecache && dnf install python
# ls -l /usr/bin/py*
lrwxrwxrwx 1 root root     8 Oct  6 14:19 /usr/bin/pydoc -> ./pydoc3
lrwxrwxrwx 1 root root     8 Oct  6 14:19 /usr/bin/pydoc3 -> pydoc3.9
-rwxr-xr-x 1 root root    78 Oct  6 14:19 /usr/bin/pydoc3.9
lrwxrwxrwx 1 root root     9 Oct  6 14:46 /usr/bin/python -> ./python3
lrwxrwxrwx 1 root root     9 Oct  6 14:19 /usr/bin/python3 -> python3.9
-rwxr-xr-x 1 root root 15536 Oct  6 14:20 /usr/bin/python3.9

[ghost]

Windows failed because of another reason: Those tests do not support to run on Windows. I have no idea why it happened

It doesn't look like the python dependencies are being installed for the Windows builds. I think the Windows build only ran the integration tests (and not the testbench/emulator tests) but since you run bazel test inside run_integration_tests_emulator_bazel.sh they're getting picked up in the Windows build. Carlos can correct me on this.

Seems like we need to install python (aka python-unversioned-command) for Fedora:33:

Thanks. It should be installed by default, odd that the docker image omits it.

[ghost]

but since you run bazel test inside run_integration_tests_emulator_bazel.sh they're getting picked up in the Windows build.

Please disregard this. It made sense in my head when I typed it.

[coryan]

Windows failed because of another reason: Those tests do not support to run on Windows. I have no idea why it happened confused

We basically run bazel test ... on Windows (and Linux for that matter), so all the unit tests try to run.

Looks like we just need to install the pip dependencies and things would work, seems like an easy change in build.bat, but I really do not have the time to investigate all the details right now.

[vnghia]

We basically run bazel test ... on Windows (and Linux for that matter), so all the unit tests try to run.

I added tags=["manual"] to the BUILD file so the tests won't be caught by ...

The CI threw /usr/bin/env: 'python': No such file or directory.

Use RUN ln -s $(which python3) /usr/bin/python seems to be the easiest way.

I also deleted some redundant (grpcio) or unused (pytest) dependencies of python in the last commit.

[coryan]

Reviewed 7 of 7 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @vnvo2409)

---

ci/kokoro/docker/Dockerfile.fedora, line 26 at r2 (raw file):

# Link `python` to `python3`
RUN ln -s $(which python3) /usr/bin/python

I think we should simply install the python package. These Dockefiles are also good documentation to setup a development environment, and we should not be recommending something like this. Also, seems that this could fail unexpectedly in the future (say because the link is automatically created)

---

ci/kokoro/docker/Dockerfile.ubuntu, line 52 at r2 (raw file):

# Link `python` to `python3`
RUN ln -s $(which python3) /usr/bin/python

Ditto.

---

google/cloud/storage/ci/run_integration_tests_emulator_bazel.sh, line 34 at r2 (raw file):

# Run the unittests of the emulator before running integration tests.
"${BAZEL_BIN}" test "${bazel_test_args[@]}" "//google/cloud/storage/emulator:test_utils" \

Can we use :all and maybe pass --test_tag_filters=manual?

---

google/cloud/storage/emulator/BUILD, line 15 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

You need to add some boilerplate I think:

package(default_visibility = ["//visibility:public"])

licenses(["notice"])  # Apache 2.0

This is still missing I believe.

---

google/cloud/storage/emulator/BUILD, line 21 at r2 (raw file):

    imports = ["."],
    main = "tests/test_gcs.py",
    tags = ["manual"],

Can you open a bug to make these unit tests run on Windows? And add a TODO(#...) - $why here?

[coryan]

+kokoro:run

[vnghia]

Can you open a bug to make these unit tests run on Windows? And add a TODO(#...) - $why here?

These tests could run perfectly on Windows. However, I feel it's noop if we run the tests without running the emulator. WDYT ?

[coryan]

Can you open a bug to make these unit tests run on Windows? And add a TODO(#...) - $why here?

These tests could run perfectly on Windows. However, I feel it's noop if we run the tests without running the emulator. WDYT ?

Yes. I was thinking we would eventually run the Windows CI build against the emulator too.

[codecov]

Codecov Report

Merging #5453 (728104a) into master (4ab7ca2) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           master    #5453    +/-   ##
========================================
  Coverage   95.49%   95.49%            
========================================
  Files        1057     1062     +5     
  Lines       97761    97984   +223     
========================================
+ Hits        93354    93573   +219     
- Misses       4407     4411     +4     

Impacted Files	Coverage Δ
google/cloud/spanner/internal/spanner_stub.cc	76.34% <0.00%> (-2.16%)	⬇️
generator/generator.cc	88.88% <0.00%> (-1.12%)	⬇️
...cloud/pubsub/internal/subscription_session_test.cc	98.97% <0.00%> (-1.03%)	⬇️
google/cloud/pubsub/benchmarks/throughput.cc	92.96% <0.00%> (-0.79%)	⬇️
google/cloud/pubsub/internal/publisher_stub.cc	96.15% <0.00%> (-0.21%)	⬇️
.../spanner/benchmarks/multiple_rows_cpu_benchmark.cc	91.80% <0.00%> (-0.11%)	⬇️
google/cloud/completion_queue_test.cc	96.85% <0.00%> (-0.10%)	⬇️
...tor/integration_tests/golden/golden_client_test.cc	98.17% <0.00%> (-0.08%)	⬇️
google/cloud/pubsub/internal/subscriber_stub.cc	98.61% <0.00%> (-0.06%)	⬇️
google/cloud/spanner/samples/samples.cc	84.62% <0.00%> (-0.06%)	⬇️
... and 31 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ab7ca2...728104a. Read the comment docs.

[coryan]

Looks good, let's see what happens with the builds +kokoro:run

Reviewed 8 of 8 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

---

google/cloud/storage/emulator/BUILD, line 21 at r2 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

Can you open a bug to make these unit tests run on Windows? And add a TODO(#...) - $why here?

Oh cool, you fix that in this PR, thanks!

[vnghia]

kokoro/macos/bazel failed due to the same reason as Windows. I would like to ask if we have to rebuild the Dockerfile.* each kokoro build or not ? If yes, I think we should move

RUN pip3 install git+git://github.com/googleapis/python-storage@8cf6c62a96ba3fff7e5028d931231e28e5029f1c
RUN pip3 install grpcio==1.32.0 flask==1.1.2 pytest==6.0.2 httpbin==0.7.0 \
     scalpl==0.4.0 crc32c==2.1 gevent==20.9.0 gunicorn==20.0.4

to storage/ci/run_integration_tests_emulator_[bazel/cmake].sh. It has some benefits:

• we don't have to rewrite these commands for each Dockerfile.*
• the dependencies will be installed only when we run the storage integration tests.
• fix kokoro/macos/bazel CI
• save some quota/resources

I will move the unit tests of the emulator to storage/ci/run_integration_tests_emulator_bazel.sh so we don't have to run the unit tests without running the emulator ( which irritates me )

---

In addition, I think we should leave Window as-is and file an issue for activating the emulator and running the unit tests at the same time which makes more sense to me.

[coryan]

In addition, I think we should leave Window as-is and file an issue for activating the emulator and running the unit tests at the same time which makes more sense to me.

That SGTM.

I would like to ask if we have to rebuild the Dockerfile.* each kokoro build or not ?

Yes, but most builds use cached versions of the docker images and refresh/rebuild the image starting from that cache.

to storage/ci/run_integration_tests_emulator_[bazel/cmake].sh

I would prefer if we refactor these commands to something like

https://github.com/googleapis/google-cloud-cpp/blob/master/ci/install-cloud-sdk.sh

Reviewable status: complete! all files reviewed, all discussions resolved

[vnghia]

Yes, but most builds use cached versions of the docker images and refresh/rebuild the image starting from that cache.

I am a little bit confused: the dependencies from pip are cached with the Dockerfile.* or they will be reinstalled in every kokoro run ? If they are cached, I think installing they inside the Dockerfile.* are more efficient than installing them separately.

[coryan]

Yes, but most builds use cached versions of the docker images and refresh/rebuild the image starting from that cache.

I am a little bit confused: the dependencies from pip are cached with the Dockerfile.* or they will be reinstalled in every kokoro run ?

Yes 😁

The builds first download a (cached) docker image created from the Dockerfile.* used in that build, let say Dockerfile.fedora-install for this example. Then they run docker build -f Dockerfile.fedora-install, but use the downloaded image as a cache source.

If nothing has changed in Dockerfile.fedora-install then the cache satisfies the image and: (a) the docker build step is very fast (as it uses the cached image), and (b) the source of the docker image cache is more reliable than downloading N dependencies from M sources (in practice).

If there are changes in Dockerfile.fedora-install then some of the steps may still use the cache (which is nice), and the changes are validated as part of the PR (which is important).

The cached docker images are updated as part of the full builds post-PR, which are not required to be fast (though it is nice if they are).

[vnghia]

After a lot of trials, I think tags=["manual"] works best with our needs. Because:

• Without the tag, these tests will run in all CI builds which use bazel ( it isn't what we want because these tests does not belong to the unit tests of the library ).
• We can control when we run the tests, on which platform.

Can we use :all and maybe pass --test_tag_filters=manual?

It does not work, we have to write all the targets manually.

Even after #5498 is closed, we should continue using this tag since it allows us to run the tests only when we need

[coryan]

just waiting for the builds. This is great news.

Reviewed 1 of 3 files at r4, 1 of 1 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved

[coryan]

Thanks again for this PR.