Fix failing test cases for proto1 -> proto2 migration #291

srinjoyray · 2024-10-22T14:54:00Z

No description provided.

ludoch · 2024-10-22T23:31:14Z

api/src/main/java/com/google/appengine/api/datastore/AsyncDatastoreServiceImpl.java

@@ -247,7 +247,7 @@ protected TransactionImpl.InternalTransaction doBeginTransaction(TransactionOpti

    Future<DatastoreV3Pb.Transaction> future =
        DatastoreApiHelper.makeAsyncCall(
-            apiConfig, DatastoreService_3.Method.BeginTransaction, request.build(), remoteTxn.build());
+            apiConfig, DatastoreService_3.Method.BeginTransaction, request.build(), remoteTxn.buildPartial());



I am still not sure we need buildPartial() in api (thinking only in test code, not code we give to customers).
So are you sure these build to buildPartial really solve test faitures?
Maybe we could change some (non public API) to use builders as long as possible, until we really need to build it)

api/src/main/java/com/google/appengine/api/datastore/DataTypeTranslator.java

api/src/main/java/com/google/appengine/api/datastore/KeyTranslator.java

ludoch · 2024-10-22T23:36:51Z

api/src/main/java/com/google/appengine/api/datastore/KeyTranslator.java

+    for(Element element : elements){
+      reversedPath.addElement(element);
+    }
+    reference.setPath(reversedPath.build());


Is there a setPathBuilder instead? Not sure.

setPathBuilder is not present

api_dev/src/test/java/com/google/appengine/api/datastore/DatastoreApiHelperTest.java

ludoch · 2024-10-28T17:13:15Z

api/src/main/java/com/google/appengine/api/datastore/DataTypeTranslator.java

@@ -258,11 +258,12 @@ private static void addListPropertyToPb(
        // If the value is indexed it appears in queries, but distinction between
        // null and empty list is lost.
      }
+      property.setValue(PropertyValue.getDefaultInstance());


Might want help from datastore team there. I am worried about the comment "// Backward compatible behavior", the logic could break moving to proto2... Also, I do not fully understand the logic of setting a default value, while the method arguments above take a collections of values.

Since value is one of the required fields, not setting it is causing build() to fail

ludoch · 2024-10-28T17:15:10Z

api/src/main/java/com/google/appengine/api/datastore/DatastoreApiHelper.java

          try {
-            responseProto.getParserForType().parseFrom(responseBytes);
+            updatedResponseProto = (T) responseProto.getParserForType().parseFrom(responseBytes);


Can you explain why we need a new variable?

I am trying to pass the response argument as a builder in this func, then we would not need another variable. But facing a bit of difficulty in handling the other functions which calls this makeAsyncCall method

ludoch · 2024-10-28T17:16:17Z

api/src/main/java/com/google/appengine/api/datastore/IndexTranslator.java

@@ -73,6 +74,7 @@ public static Index.Property convertFromPb(OnestoreEntity.Index.Property propert
  }

  public static Index convertFromPb(OnestoreEntity.Index index) {
-    return convertFromPb(OnestoreEntity.CompositeIndex.newBuilder().setId(0).setDefinition(index).build());
+    return convertFromPb(OnestoreEntity.CompositeIndex.newBuilder().setId(0).setDefinition(index).setAppId("").setState(


Hum, sure an emtpy appId is fine there? Maybe, but not sure!

I'm using the values that are present in the private default constructor

ludoch · 2024-10-28T17:18:27Z

api/src/main/java/com/google/appengine/api/datastore/QueryTranslator.java

@@ -204,13 +205,15 @@ private static Filter convertFilterPredicateToPb(Query.FilterPredicate predicate
        filterPb
            .addPropertyBuilder()
            .setName(predicate.getPropertyName())
-            .setValue(DataTypeTranslator.toV3Value(value));
+            .setValue(DataTypeTranslator.toV3Value(value))
+            .setMultiple(false);


Safe to be false? Add a comment.

api_dev/src/test/java/com/google/appengine/api/datastore/CursorTest.java

* Copybara import of the project: -- bbad663 by Mend Renovate <[email protected]>: Update all non-major dependencies COPYBARA_INTEGRATE_REVIEW=#285 from renovate-bot:renovate/all-minor-patch bbad663 PiperOrigin-RevId: 684477251 Change-Id: If7dfb2beb25d1378618a884350ec5d8d4bd3dd46 * Copybara import of the project: -- af7d544 by Mend Renovate <[email protected]>: Update all non-major dependencies COPYBARA_INTEGRATE_REVIEW=#286 from renovate-bot:renovate/all-minor-patch af7d544 PiperOrigin-RevId: 685680510 Change-Id: I96df12dbd2fb7c6eb6cfaae3489626a14766bef9 * Upgrade GAE Java version from 2.0.30 to 2.0.31 and prepare next version 2.0.32-SNAPSHOT PiperOrigin-RevId: 686867662 Change-Id: Id02da08eb160552da419cace43b86c3558e08b0d * Update all non-major dependencies * Update all non-major dependencies * Fix multithread build by ensuring jars are build correctly before copy those jars Signed-off-by: Olivier Lamy <[email protected]> * Update dependency com.google.cloud:google-cloud-logging to v3.20.5 * Simplity copying dependencies Signed-off-by: Olivier Lamy <[email protected]> * exclude not needed artifacts Signed-off-by: Olivier Lamy <[email protected]> * Do not include runtime-* in WEB-INF/lib Signed-off-by: Olivier Lamy <[email protected]> * Copybara import of the project: -- cb49a97 by Mend Renovate <[email protected]>: Update all non-major dependencies COPYBARA_INTEGRATE_REVIEW=#299 from renovate-bot:renovate/all-minor-patch cb49a97 PiperOrigin-RevId: 690485840 Change-Id: I76205310268bc27e3c1ab56be4d42b6d486f4e4c * Copybara import of the project: -- 05d0d3c by Mend Renovate <[email protected]>: Update all non-major dependencies COPYBARA_INTEGRATE_REVIEW=#299 from renovate-bot:renovate/all-minor-patch 05d0d3c PiperOrigin-RevId: 690870869 Change-Id: I1ba450e60d8637dfd64adb866a5b185b08a744ad * Update all non-major dependencies --------- Signed-off-by: Olivier Lamy <[email protected]> Co-authored-by: Mend Renovate <[email protected]> Co-authored-by: Srinjoy Ray <[email protected]> Co-authored-by: GAE Java Team <[email protected]> Co-authored-by: Olivier Lamy <[email protected]>

…ing shading...

srinjoyray added 11 commits October 22, 2024 20:13

tests fix

556f908

removed extra dependency

9c2191f

wip

a1d5091

wip

5f2586c

fixed testMultiValuePropertySort

82c3b85

fixed test cases

a31833f

wip - fixing test cases

563e309

used getDefaultInstance()

4a9e891

high level changes

e3f4cf2

wip

dc4c3f1

wip

b58db82

ludoch reviewed Oct 28, 2024

View reviewed changes

srinjoyray and others added 9 commits October 29, 2024 00:16

review fix

2c718f9

wip fixing tests

6b973ca

wip - fixed DatastoreCallbacksTests

d770e51

wip

15c6e84

wip - fixed nextCall

889e555

fixed tests

1f5fbe1

Fix regression that renamed the same proto in 2 different classes dur…

0db04e3

…ing shading...

srinjoyray changed the title ~~[Draft] Fix failing test cases for proto1 -> proto2 migration~~ Fix failing test cases for proto1 -> proto2 migration Nov 9, 2024

Merge branch 'proto2' into proto2-tests

1358e17

srinjoyray merged commit c40dbca into proto2 Nov 9, 2024
3 of 4 checks passed

srinjoyray deleted the proto2-tests branch November 9, 2024 05:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix failing test cases for proto1 -> proto2 migration #291

Fix failing test cases for proto1 -> proto2 migration #291

srinjoyray commented Oct 22, 2024

ludoch Oct 22, 2024

ludoch Oct 22, 2024

srinjoyray Oct 28, 2024

ludoch Oct 28, 2024

srinjoyray Oct 30, 2024

ludoch Oct 28, 2024

srinjoyray Oct 28, 2024

ludoch Oct 28, 2024

srinjoyray Oct 28, 2024

ludoch Oct 28, 2024

Fix failing test cases for proto1 -> proto2 migration #291

Fix failing test cases for proto1 -> proto2 migration #291

Conversation

srinjoyray commented Oct 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment