feat: improve ta_storage.bigquery #1017
Conversation
joseph-sentry
commented
Jan 20, 2025
joseph-sentry
commented
- generalize the way bigquery accepts query parameters
- change type of params arg in bigquery_service to sequence
- feat: add upload_id field to ta_testrun protobuf
- add flags_hash field to ta_testrun protobuf
- create new testid generation function
- add test_id to ta_testrun proto
- add flaky_failure to testrun protobuf
- handle flaky failures in ta_storage.bq
- create sql queries for reading from bq
- write tests for ta_storage.bq aggregate queries
- generalize the way bigquery accepts query parameters - change type of params arg in bigquery_service to sequence - feat: add upload_id field to ta_testrun protobuf - add flags_hash field to ta_testrun protobuf - create new testid generation function - add test_id to ta_testrun proto - add flaky_failure to testrun protobuf - handle flaky failures in ta_storage.bq - create sql queries for reading from bq - write tests for ta_storage.bq aggregate queries
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
📢 Thoughts on this report? Let us know! |
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #1017 +/- ##
==========================================
- Coverage 97.78% 97.54% -0.24%
==========================================
Files 451 452 +1
Lines 36913 37064 +151
==========================================
+ Hits 36095 36155 +60
- Misses 818 909 +91
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
✅ All tests successful. No failed tests were found. 📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
| @@ -27,4 +28,8 @@ message TestRun { | |||
|
|
|||
| optional string filename = 14; | |||
| optional string framework = 15; | |||
|
|
|||
| optional int64 upload_id = 16; | |||
There was a problem hiding this comment.
is there a specific reason why all the fields ar optional, is this for forward compatiblity reasons?
There was a problem hiding this comment.
protobuf generally prefers fields to be optional, if a field is marked as required then it's required forever: context
|
|
||
|
|
||
| def calc_flags_hash(flags: list[str]) -> bytes | None: | ||
| flags_str = " ".join(flags) # we know that flags cannot contain spaces |
There was a problem hiding this comment.
| flags_str = " ".join(flags) # we know that flags cannot contain spaces | |
| flags_str = " ".join(sorted(flags)) # we know that flags cannot contain spaces |
just to make sure sort order is consistent. not sure if that might already be guaranteed from the caller?
| # we only need the first one | ||
| flags_hash, _ = mmh3.hash64(flags_str) | ||
| try: | ||
| flags_hash_bytes = flags_hash.to_bytes(8, byteorder) |
There was a problem hiding this comment.
you should use a fixed byteorder instead of using the system one.
| try: | ||
| flags_hash_bytes = flags_hash.to_bytes(8, byteorder) | ||
| return flags_hash_bytes | ||
| except OverflowError as e: # this should never happen because hash64 should always return 2 64 bit ints |
There was a problem hiding this comment.
I think OverflowError is the wrong exception (according to docs https://docs.python.org/3/library/exceptions.html#OverflowError) in case of destructuring assignments.
As the comment says, if this ever fails, its a bug in the underlying package, so I’m okay with just not catching this.
