Pin locale to deal with flakyness in GCS testing library - #4869
Open
epugh wants to merge 2 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The implementation skips tests instead of pinning the locale as described and contains null-unsafe exception matching.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Mitigates locale-related failures in the in-memory GCS test library.
Changes:
- Skips tests when the known date-parsing failure occurs.
- Rethrows unrelated failures.
- Adds guarded blob creation for two copy tests.
File summaries
| File | Description |
|---|---|
LocalStorageGCSBackupRepository.java |
Handles failures during test repository initialization. |
GCSBackupRepositoryTest.java |
Adds locale-failure handling when creating test blobs. |
Review details
Suppressed comments (1)
solr/modules/gcs-repository/src/test/org/apache/solr/gcs/GCSBackupRepositoryTest.java:182
- If a different
StorageExceptionhas aNumberFormatExceptioncause with no message, this predicate throwsNullPointerExceptioninstead of rethrowing the genuine failure below. Guard the message before callingcontains.
cause != null
&& e instanceof StorageException
&& cause.getMessage().contains("Invalid date/time format")
&& cause instanceof NumberFormatException);
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+177
to
+182
| 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); |
Comment on lines
+101
to
+104
| cause != null | ||
| && e instanceof StorageException | ||
| && cause.getMessage().contains("Invalid date/time format") | ||
| && cause instanceof NumberFormatException); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Solr randomized-locale test picked dz (Dzongkha), and Google's FakeStorageRpc (the in-memory GCS test double behind LocalStorageHelper) formats its own internal "now" timestamp using the JVM default locale without forcing ASCII digits. Under dz that produces Tibetan numerals (༢༠༢༦-༠༩-༠༣T༡༢:༥༩:༤༣Z = "2026-09-03T12:59:43Z"), which its own strict RFC3339 parser then can't read back — NumberFormatException → StorageException. This is a bug in the third-party test double, not in Solr's GCS repository code
Solution
Pin the locale to root.
Tests
These tests were failing on main: