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

test(spanner): fix the failed TestColumnTypeErr test #4450

Merged
merged 3 commits into from Jul 19, 2021

Conversation

hengfengli
Copy link
Contributor

Fix #4443

@hengfengli hengfengli added the api: spanner Issues related to the Spanner API. label Jul 16, 2021
@hengfengli hengfengli self-assigned this Jul 16, 2021
@hengfengli hengfengli requested review from skuruppu and a team as code owners July 16, 2021 03:25
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 16, 2021
@hengfengli
Copy link
Contributor Author

@olavloite I wonder why the previous CI tests did not catch this error. It may be due to caching the test results.

@olavloite
Copy link
Contributor

@olavloite I wonder why the previous CI tests did not catch this error. It may be due to caching the test results.

Yes, that is very strange. I looked into the log of the build for the PR, and the test failed in that build as well, but the build is marked as green: https://source.cloud.google.com/results/invocations/1e012eba-5a8c-4681-9a59-919f7ec7e1d3/targets/cloud-devrel%2Fclient-libraries%2Fgo%2Fgoogle-cloud-go%2Fpresubmit%2Fgo111/log (search for TestColumnTypeErr in the log output).
It worries me that the CI can mark a build as green while there are test failures. Do you know who could best look into that?

@olavloite
Copy link
Contributor

@hengfengli If you take a look at the Kokoro build log for this build as well, you will see that there is a test failure there as well:

=== RUN   TestRowToString
--- FAIL: TestRowToString (0.00s)
    row_test.go:1693: got {fields: [name:"F1"  type:{code:STRING} name:"F2"  type:{code:STRING}], values: [string_value:"v1" string_value:"v2"]}, want {fields: [name:"F1" type:{code:STRING} name:"F2" type:{code:STRING}], values: [string_value:"v1" string_value:"v2"]}

@codyoss Is this a known issue? We are getting green builds from kokoro while there are test failures.

@olavloite olavloite added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 16, 2021
@hengfengli
Copy link
Contributor Author

hengfengli commented Jul 16, 2021

@hengfengli If you take a look at the Kokoro build log for this build as well, you will see that there is a test failure there as well:

=== RUN   TestRowToString
--- FAIL: TestRowToString (0.00s)
    row_test.go:1693: got {fields: [name:"F1"  type:{code:STRING} name:"F2"  type:{code:STRING}], values: [string_value:"v1" string_value:"v2"]}, want {fields: [name:"F1" type:{code:STRING} name:"F2" type:{code:STRING}], values: [string_value:"v1" string_value:"v2"]}

@olavloite It is the whitespaces between name and type. Sometime, it gets two spaces and sometimes it gets a single space. I tested it locally and the test passes. Do you have any suggestion on how to verify this? Or, maybe I can just delete this test.

@olavloite
Copy link
Contributor

olavloite commented Jul 16, 2021

@hengfengli If you take a look at the Kokoro build log for this build as well, you will see that there is a test failure there as well:

=== RUN   TestRowToString
--- FAIL: TestRowToString (0.00s)
    row_test.go:1693: got {fields: [name:"F1"  type:{code:STRING} name:"F2"  type:{code:STRING}], values: [string_value:"v1" string_value:"v2"]}, want {fields: [name:"F1" type:{code:STRING} name:"F2" type:{code:STRING}], values: [string_value:"v1" string_value:"v2"]}

@olavloite It is the whitespaces between name and type. Sometime, it gets two spaces and sometimes it gets a single space. I tested it locally and the test passes. Do you have any suggestion on how to verify this? Or, maybe I can just delete this test.

@hengfengli It's caused by this line which will add an additional space based on a deterministically random boolean value. So you could call if detrand.Bool() in the test to determine whether the string will/should contain 2 spaces or not.

@codyoss
Copy link
Member

codyoss commented Jul 16, 2021

@olavloite Thanks for bringing this to my attention this is a bug I was unaware of. Will try to get out a fix today.

codyoss added a commit that referenced this pull request Jul 16, 2021
)

Emulator tests were overwritting the results of the main test
run which can cause errors that occured in main test run not
to be reported when the emulator tests pass. Now emulator tests
append to the test log.

Updates: #4450
@olavloite olavloite removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 16, 2021
@hengfengli
Copy link
Contributor Author

if detrand.Bool()

@olavloite "google.golang.org/protobuf/internal/detrand" is an internal package. I can't use it in my code. I did it in a different way, PTAL.

@olavloite
Copy link
Contributor

if detrand.Bool()

@olavloite "google.golang.org/protobuf/internal/detrand" is an internal package. I can't use it in my code. I did it in a different way, PTAL.

Sorry, I didn't notice that it was an internal package. Your alternative solution looks good to me.

@hengfengli
Copy link
Contributor Author

@skuruppu Can I get an approval from you? Thanks.

Copy link
Member

@codyoss codyoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@codyoss
Copy link
Member

codyoss commented Jul 19, 2021

Merging this to fix CI.

@codyoss codyoss merged commit 1110dcf into googleapis:master Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the Spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spanner: TestColumnTypeErr failed
3 participants