Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Check mutation error only if exists #231

Closed
wants to merge 1 commit into from

Conversation

bhshkh
Copy link
Collaborator

@bhshkh bhshkh commented Jan 15, 2025

TestMutateRows_Generic_DeadlineExceeded fails for Go client library with below error

=== RUN   TestMutateRows_Generic_DeadlineExceeded
[Servr log] 2025/01/15 19:19:42 There is 10s sleep on the server side
2025/01/15 19:19:44 received error from Table.ApplyBulk(): rpc error: code = DeadlineExceeded desc = context deadline exceeded
2025/01/15 19:19:44 error: rpc error: code = DeadlineExceeded desc = context deadline exceeded
    mutaterows_test.go:227: 
        	Error Trace:	/home/runner/work/google-cloud-go/google-cloud-go/cloud-bigtable-clients-test/tests/mutaterows_test.go:227
        	Error:      	Not equal: 
        	            	expected: 1
        	            	actual  : 0
        	Test:       	TestMutateRows_Generic_DeadlineExceeded
--- FAIL: TestMutateRows_Generic_DeadlineExceeded (2.00s)

This test expects overall operation to return error and also per mutation error but in Go, method is documented as below:

// If the entire process
// fails, (nil, err) will be returned. If specific mutations
// fail to apply, ([]err, nil) will be returned, and the errors
// will correspond to the relevant rowKeys/muts arguments.

So, I can't return ([]err, err) from Go library ApplyBulk method. This may break user's code.

Thus, updating the test itself

@bhshkh bhshkh requested a review from mutianf January 15, 2025 19:30
@andre-sampaio
Copy link

I'm rethinking what I suggested about changing the test: now that I think about it this could make other clients more brittle.

I think it would be safer to change the test-proxy implementation for the go client.

Also, I have created an issue for us to review ApplyBulk at some point: googleapis/google-cloud-go#11473

@bhshkh
Copy link
Collaborator Author

bhshkh commented Jan 21, 2025

Created googleapis/google-cloud-go#11478 to update test proxy

@bhshkh bhshkh closed this Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants