ennuite commented on PR #732: URL: https://github.com/apache/arrow-go/pull/732#issuecomment-5211658281
Still not ready for a review as I still need to look into the protobuf stuff and the release notes, but a question popped in my mind (which is probably nitpicky). What is your opinion on `s.Require.False(`) vs `s.False()`? I haven't found a canonical way to handle these (although I understand in how their behavior differs). For the server tests, I'm always using `Require()` for every check of values returned by `PreparedStatement.IsUpdate()`, because the code after those checks (if there is any) just execute the query, and without proper values returned by IsUpdate we don't know how to properly execute the query (DoPut vs DoGet) For the client tests: 1. In my new tests (`TestPreparedStatementExecuteWithIsUpdateFalse` and `TestPreparedStatementExecuteUpdateWithIsUpdateTrue`) I also use `Require()` using the same logic as above. 2. For `TestPreparedStatementLoadFromResultWithIsUpdate`: given that we are just testing the loading and not execution, it would be interesting to see if `ok` was loaded incorrectly and `val` correctly, so I don't use `Require()` 3. For the pre-existing tests I don't use `Require` because there are interesting things to test even if these fields are broken (the tests were not created solely for this feature). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
