Copilot commented on code in PR #4869:
URL: https://github.com/apache/solr/pull/4869#discussion_r3925386708
##########
solr/modules/gcs-repository/src/test/org/apache/solr/gcs/GCSBackupRepositoryTest.java:
##########
@@ -163,6 +164,27 @@ public void testCopyIndexFileToCopiesFile() throws
Exception {
}
}
+ /**
+ * Creates a blob, skipping (rather than failing) the test if the current
default locale trips the
+ * known FakeStorageRpc/RFC3339 date-parsing bug - see {@link
+ * LocalStorageGCSBackupRepository#initializeBackupLocation()} for the same
pattern.
+ */
+ private static void createBlob(Storage storage, BlobId blobId, byte[] data) {
+ try {
+ storage.create(BlobInfo.newBuilder(blobId).build(), data);
+ } catch (Exception e) {
+ final Throwable cause = e.getCause();
+ Assume.assumeFalse(
+ "This test uses a GCS mock library that is incompatible with the
current default locale",
+ cause != null
+ && e instanceof StorageException
+ && cause.getMessage().contains("Invalid date/time format")
+ && cause instanceof NumberFormatException);
Review Comment:
The PR says this fix pins the locale to `Locale.ROOT`, but this path instead
turns the affected tests into assumptions, so the reported `dz` seed no longer
executes either copy assertion. Please pin and restore the locale around the
fake-storage tests (including users of `LocalStorageGCSBackupRepository`) if
that is the intended fix, rather than silently reducing coverage; otherwise the
PR description and title need to reflect the skip-based behavior.
This issue also appears on line 179 of the same file.
##########
solr/modules/gcs-repository/src/test/org/apache/solr/gcs/LocalStorageGCSBackupRepository.java:
##########
@@ -96,13 +96,14 @@ protected void initializeBackupLocation() {
createDirectory(baseLocationUri);
} catch (Exception e) {
final Throwable cause = e.getCause();
- if (cause != null) {
- assumeFalse(
- "This test uses a GCS mock library that is incompatible with the
current default locale",
- e instanceof StorageException
- && cause.getMessage().contains("Invalid date/time format")
- && cause instanceof NumberFormatException);
- }
+ assumeFalse(
+ "This test uses a GCS mock library that is incompatible with the
current default locale",
+ cause != null
+ && e instanceof StorageException
+ && cause.getMessage().contains("Invalid date/time format")
+ && cause instanceof NumberFormatException);
Review Comment:
A non-locale `StorageException` whose `NumberFormatException` cause has a
null message is replaced by a new `NullPointerException` here, contradicting
the intent to preserve genuine failures. Check the cause type and message for
null before calling `contains`.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
