test: Add conformance tests #1349

danieljbruce · 2023-10-03T20:52:10Z

Add conformance tests that use the test proxy to ensure the client is running correctly. This PR should be very similar to https://github.com/googleapis/java-bigtable/pull/1699 which adds conformance tests to java-bigtable. A list of known test failures is also provided so that we only test against the test proxy tests that pass and our continuous integration pipeline passes. The idea is that we want to reduce this list over time as the test proxy improves.

Add conformance tests that use the test proxy to ensure the client is running correctly.

conventional-commit-lint-gcf · 2023-10-03T20:52:15Z

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

generated-files-bot · 2023-10-03T20:52:16Z

Warning: This pull request is touching the following templated files:

.kokoro/conformance.sh - .kokoro files are templated and should be updated in synthtool

This reverts commit aa18af6.

If the yaml config sets up node then the node test proxy can be tested.

This reverts commit 2bb0eed.

This reverts commit 80d5569.

This reverts commit 359d413.

Add execution permissions to the conformance shell file.

Add &

The path for the known failures is wrong and needs to be corrected.

Errors out because skip has no argument

The proxy address needs to be set to where the test proxy is listening

The known failures should be added to the file so that those tests can be skipped

By putting all the test names on one line maybe they will all be picked up to be skipped rather than just one of them.

Run the linter

daniel-sanche · 2023-10-05T20:20:59Z

.github/workflows/conformance.yaml

+  conformance:
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v3


Is this needed? Looks like a duplicate of the step below

I bet it's not needed, but I'm not completely sure. The PR in Java has this twice so I figured there must be a reason it is there https://github.com/googleapis/java-bigtable/pull/1699/files. I'll investigate further.

I've been informed by @liujiongxin this check is probably for the Java client repo itself.

Right, that makes sense. I forgot there were two repos involved!

daniel-sanche · 2023-10-05T20:21:59Z

.github/workflows/conformance.yaml

+        path: cloud-bigtable-clients-test
+    - uses: actions/setup-node@v3
+      with:
+        node-version: 14


Do you think it would be useful to test with multiple node versions?

daniel-sanche · 2023-10-05T20:26:20Z

testproxy/known_failures.txt

@@ -0,0 +1 @@
+TestCheckAndMutateRow_NoRetry_TransientError\|TestCheckAndMutateRow_Generic_Headers\|TestCheckAndMutateRow_Generic_DeadlineExceeded\|TestMutateRow_Generic_Headers\|TestMutateRow_Generic_DeadlineExceeded\|TestMutateRows_Generic_CloseClient\|TestReadModifyWriteRow_Generic_Headers\|TestReadModifyWriteRow_NoRetry_TransientError\|TestReadModifyWriteRow_Generic_DeadlineExceeded\|TestReadRow_Generic_DeadlineExceeded\|TestReadRows_Retry_PausedScan\|TestReadRows_Retry_StreamReset\|TestMutateRows_Generic_DeadlineExceeded\|TestSampleRowKeys_Generic_Headers\|TestReadRows_Generic_DeadlineExceeded\|TestReadRows_Generic_CloseClient\|TestSampleRowKeys_Generic_DeadlineExceeded


I'm a little unsure if we should be skipping all these tests, maybe we should just let the test stay red until these are fixed?

Let's look into this further.

We should have two test paths: default and all tests enabled.

danieljbruce

Answer to review comments.

danieljbruce · 2023-10-05T20:43:33Z

.github/workflows/conformance.yaml

+  conformance:
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v3


I bet it's not needed, but I'm not completely sure. The PR in Java has this twice so I figured there must be a reason it is there https://github.com/googleapis/java-bigtable/pull/1699/files. I'll investigate further.

danieljbruce · 2023-10-05T20:43:57Z

.github/workflows/conformance.yaml

+        path: cloud-bigtable-clients-test
+    - uses: actions/setup-node@v3
+      with:
+        node-version: 14


danieljbruce · 2023-10-05T20:44:30Z

testproxy/known_failures.txt

@@ -0,0 +1 @@
+TestCheckAndMutateRow_NoRetry_TransientError\|TestCheckAndMutateRow_Generic_Headers\|TestCheckAndMutateRow_Generic_DeadlineExceeded\|TestMutateRow_Generic_Headers\|TestMutateRow_Generic_DeadlineExceeded\|TestMutateRows_Generic_CloseClient\|TestReadModifyWriteRow_Generic_Headers\|TestReadModifyWriteRow_NoRetry_TransientError\|TestReadModifyWriteRow_Generic_DeadlineExceeded\|TestReadRow_Generic_DeadlineExceeded\|TestReadRows_Retry_PausedScan\|TestReadRows_Retry_StreamReset\|TestMutateRows_Generic_DeadlineExceeded\|TestSampleRowKeys_Generic_Headers\|TestReadRows_Generic_DeadlineExceeded\|TestReadRows_Generic_CloseClient\|TestSampleRowKeys_Generic_DeadlineExceeded


Let's look into this further.

The conformance tests may catch more issues to do with streaming and such if more versions of node are covered.

daniel-sanche · 2023-10-05T21:43:50Z

.kokoro/conformance.sh

+cd $(dirname $0)/..
+
+# Build and start the proxy in a separate process
+pushd .


The pushd and popd shouldn't be necessary if you stay at the root

What exactly would you suggest here if we eliminate pushd and popd? I tried subshells locally https://unix.stackexchange.com/questions/13802/execute-a-specific-command-in-a-given-directory-without-cding-to-it, but the command hangs.

Looking at this further, I think in the Java example we wanted to go back to the root because we wanted to run https://github.com/googleapis/java-bigtable/pull/1699/files#diff-08b007a16e10891ada78edba8173c7323e74500fa6e1600c5e1dc7a721df5388R62. Since we don't need to run anything else in this script after the golang tests however, I think we can eliminate pushd . and popd and not worry about staying at the root.

pushd and popd are only needed if you're changing directories. In the case of this specific block , it seems like a no-op, so yeah it makes sense to remove it

Alternatively, you could replace all the cd statements with pushd statements, which can be a lot cleaner if you have to move around the filesystem. But since this script is pretty simple, you can probably just stick with the cds you have

.kokoro/conformance.sh

daniel-sanche · 2023-10-05T22:18:24Z

.kokoro/conformance.sh

+# Run the conformance test
+pushd .
+pwd
+ls


was pwd and ls added for debugging? Can probably remove them

Yes. These debugging commands have been removed now.

pwd and ls should not be left in the script

These two commands are unnecessary as nothing is run after them.

bhshkh · 2023-10-08T22:06:24Z

.kokoro/conformance.sh

+RETURN_CODE=$?
+
+# Stop the proxy
+kill $proxyPID


This should be executed irrespective of when / how script exits. Exit traps can be used here. E.g. https://github.com/googleapis/google-cloud-go/blob/5de394be14cf852b99dde65fbeba3d02714f6845/storage/emulator_test.sh#L64-L70

Sure. Makes sense. So would you suggest adding trap kill $proxyPID somewhere at the top of the file and removing this line? In that case would any exit signal still get propogated up to the user?

Add a cleanup function.
Trap on EXIT signal which would catch any exit signal.

Similar to https://github.com/googleapis/google-cloud-go/blob/5de394be14cf852b99dde65fbeba3d02714f6845/storage/emulator_test.sh#L70

Was the trap change pushed? I don't see the changes here

In my git client at about 1:30 I made the changes and pushed them. Not sure why they don't show up.

It is called "Add a cleanup operation"

As required, remove the list of known failures

The cleanup operation should happen no matter how the program exits.

Add conformance tests

1ba08e6

Add conformance tests that use the test proxy to ensure the client is running correctly.

danieljbruce requested review from a team as code owners October 3, 2023 20:52

product-auto-label bot added the size: m Pull request size is medium. label Oct 3, 2023

product-auto-label bot added the api: bigtable Issues related to the googleapis/nodejs-bigtable API. label Oct 3, 2023

danieljbruce added 2 commits October 3, 2023 16:59

try this and see what it does

aa18af6

Revert "try this and see what it does"

a4e5c47

This reverts commit aa18af6.

danieljbruce marked this pull request as draft October 3, 2023 21:04

danieljbruce added 20 commits October 3, 2023 17:10

Make the yaml config run node and not java

a91f25b

If the yaml config sets up node then the node test proxy can be tested.

Add echo to test

11cd798

Add conformance to main CI pipeline instead

359d413

Add execution permissions

80d5569

typo correction

2bb0eed

Revert "typo correction"

bfddf98

This reverts commit 2bb0eed.

Revert "Add execution permissions"

d51206f

This reverts commit 80d5569.

Revert "Add conformance to main CI pipeline instead"

a0dab58

This reverts commit 359d413.

After reverts, rearrange files for best setup

16c796c

Add execution permissions to the conformance shell file.

Add logs to see what it does

fccb1ef

Try adding more logs

5b49bd3

Make sure it doesn’t get stuck

8b2762a

Add &

Correct conformance paths test run path

63cca41

The path for the known failures is wrong and needs to be corrected.

Try without setting the proxy address

df1d9d9

Fix path to the known failures again

d5d0281

Add a known failure so test will work

055e82c

Errors out because skip has no argument

Set the proxy address

b0be854

The proxy address needs to be set to where the test proxy is listening

Add known failures based on tests that fail

207e0c8

The known failures should be added to the file so that those tests can be skipped

Put all the test names on one line

07c9d60

By putting all the test names on one line maybe they will all be picked up to be skipped rather than just one of them.

Try using an ampersand to separate parameters

cb9f89d

danieljbruce added 2 commits October 4, 2023 15:46

run the linter

682431e

Run the linter

remove console logs

3a2d680

danieljbruce marked this pull request as ready for review October 5, 2023 14:27

Merge branch 'main' into conformance-tests

1586be1

bhshkh approved these changes Oct 5, 2023

View reviewed changes

danieljbruce added the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 5, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 5, 2023

daniel-sanche approved these changes Oct 5, 2023

View reviewed changes

danieljbruce commented Oct 5, 2023

View reviewed changes

Test on multiple versions of node

a798431

The conformance tests may catch more issues to do with streaming and such if more versions of node are covered.

danieljbruce added the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 5, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 5, 2023

danieljbruce requested a review from liujiongxin October 5, 2023 21:35

daniel-sanche reviewed Oct 5, 2023

View reviewed changes

.kokoro/conformance.sh Show resolved Hide resolved

daniel-sanche reviewed Oct 5, 2023

View reviewed changes

danieljbruce added 2 commits October 6, 2023 10:01

Eliminate logging for debugging

fd430be

pwd and ls should not be left in the script

Remove pushd and popd

7d5855e

These two commands are unnecessary as nothing is run after them.

bhshkh reviewed Oct 8, 2023

View reviewed changes

danieljbruce mentioned this pull request Oct 10, 2023

chore(tests): add conformance tests to CI for v3 googleapis/python-bigtable#870

Merged

danieljbruce added 2 commits October 10, 2023 15:13

Remove known failures list of test

8fb6e13

As required, remove the list of known failures

Add a cleanup operation

dcb3c9c

The cleanup operation should happen no matter how the program exits.

danieljbruce added the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 11, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 11, 2023

danieljbruce mentioned this pull request Oct 11, 2023

test(bigtable): Run conformance tests for bigtable googleapis/google-cloud-go#8658

Merged

triplequark approved these changes Oct 11, 2023

View reviewed changes

Empty-Commit

6a381e4

danieljbruce added the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 12, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 12, 2023

danieljbruce merged commit 5cc0523 into googleapis:main Oct 12, 2023
15 of 19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Add conformance tests #1349

test: Add conformance tests #1349

danieljbruce commented Oct 3, 2023 •

edited

Loading

conventional-commit-lint-gcf bot commented Oct 3, 2023 •

edited

Loading

generated-files-bot bot commented Oct 3, 2023 •

edited

Loading

daniel-sanche Oct 5, 2023

danieljbruce Oct 5, 2023

danieljbruce Oct 5, 2023

daniel-sanche Oct 5, 2023

daniel-sanche Oct 5, 2023

danieljbruce Oct 5, 2023

daniel-sanche Oct 5, 2023

danieljbruce Oct 5, 2023

triplequark Oct 5, 2023

danieljbruce left a comment

danieljbruce Oct 5, 2023

danieljbruce Oct 5, 2023

danieljbruce Oct 5, 2023

daniel-sanche Oct 5, 2023

danieljbruce Oct 6, 2023

daniel-sanche Oct 11, 2023

daniel-sanche Oct 5, 2023

danieljbruce Oct 6, 2023

bhshkh Oct 8, 2023

danieljbruce Oct 10, 2023

bhshkh Oct 11, 2023

danieljbruce Oct 11, 2023

daniel-sanche Oct 11, 2023

danieljbruce Oct 11, 2023

danieljbruce Oct 11, 2023

		@@ -0,0 +1 @@
		TestCheckAndMutateRow_NoRetry_TransientError\\|TestCheckAndMutateRow_Generic_Headers\\|TestCheckAndMutateRow_Generic_DeadlineExceeded\\|TestMutateRow_Generic_Headers\\|TestMutateRow_Generic_DeadlineExceeded\\|TestMutateRows_Generic_CloseClient\\|TestReadModifyWriteRow_Generic_Headers\\|TestReadModifyWriteRow_NoRetry_TransientError\\|TestReadModifyWriteRow_Generic_DeadlineExceeded\\|TestReadRow_Generic_DeadlineExceeded\\|TestReadRows_Retry_PausedScan\\|TestReadRows_Retry_StreamReset\\|TestMutateRows_Generic_DeadlineExceeded\\|TestSampleRowKeys_Generic_Headers\\|TestReadRows_Generic_DeadlineExceeded\\|TestReadRows_Generic_CloseClient\\|TestSampleRowKeys_Generic_DeadlineExceeded

test: Add conformance tests #1349

test: Add conformance tests #1349

Conversation

danieljbruce commented Oct 3, 2023 • edited Loading

conventional-commit-lint-gcf bot commented Oct 3, 2023 • edited Loading

generated-files-bot bot commented Oct 3, 2023 • edited Loading

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

danieljbruce 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

danieljbruce commented Oct 3, 2023 •

edited

Loading

conventional-commit-lint-gcf bot commented Oct 3, 2023 •

edited

Loading

generated-files-bot bot commented Oct 3, 2023 •

edited

Loading