chore(entity-query-service): better integration tests for update API #128
Conversation
Codecov Report
@@ Coverage Diff @@
## main #128 +/- ##
============================================
+ Coverage 59.71% 60.46% +0.75%
Complexity 303 303
============================================
Files 39 39
Lines 3043 3043
Branches 376 376
============================================
+ Hits 1817 1840 +23
+ Misses 1053 1028 -25
- Partials 173 175 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -734,4 +788,10 @@ private static Datastore getDatastore() { | |||
|
|
|||
| return attributesMap; | |||
| } | |||
|
|
|||
| private <T> void assertListEquals(List<T> expected, List<T> actual) { | |||
There was a problem hiding this comment.
why can't this just use assertEquals?
There was a problem hiding this comment.
I tried that. It wasn't working with the basic assertEquals, though it should have
Didn't spend much time on why it wasn't working
There was a problem hiding this comment.
IMO, worth figuring out - something's off here. This check for example doesn't assert that their order matches, which is part of list equality.
| @@ -484,6 +489,11 @@ public void testBulkUpdate() { | |||
| .setTenantId(TENANT_ID) | |||
| .setEntityType(EntityType.API.name()) | |||
| .setEntityName("api1") | |||
| .putAttributes( | |||
There was a problem hiding this comment.
can we add an integer to the integration test, too?
There was a problem hiding this comment.
We don't support updating INT yet via EQS update APIs. Planning to add support for that in a separate PR
There was a problem hiding this comment.
we don't? maybe I'm misunderstanding or looking in the wrong spot, but it sure looks like we do - unless it's in the API but not the implementation?
message UpdateOperation {
oneof operation {
SetAttribute setAttribute = 1;
// more update operations in the future
}
}->
message SetAttribute {
ColumnIdentifier attribute = 1;
LiteralConstant value = 2;
}->
message LiteralConstant {
Value value = 1;
}->
message Value {
ValueType valueType = 1;
string string = 3;
int64 long = 4;
int32 int = 5;
float float = 6;
double double = 7;
bytes bytes = 8;
bool boolean = 9;
sfixed64 timestamp = 15;
repeated string string_array = 16;
repeated int64 long_array = 17;
repeated int32 int_array = 18;
repeated float float_array = 19;
repeated double double_array = 20;
repeated bytes bytes_array = 21;
repeated bool boolean_array = 22;
map<string, string> string_map = 23;
}There was a problem hiding this comment.
oh, the implementation only has long in its switch, not int. That's really confusing, but is actually right (other than the fact that it should yell loudly and not have it in its interface)- entities only have long fields, not int.
...and just to clarify further, because this is a complete mess - Entity the object returned by EDS can actually hold both int and long, and Row the EQS response can hold both (and those two don't match WRT certain other data types 😢 ), but our attribute schema, which represents the entity field data types does not have an int type - and our attribute schema should be the source of truth here.
So tl;dr - we shouldn't add support for int, but could we add long to the test?
No description provided.