Snow-1016470: increase code coverage #2011
Conversation
| injectedException = th; | ||
| } | ||
|
|
||
| public static boolean isInjectedExceptionEnabled() { |
There was a problem hiding this comment.
This shouldn't be public
| @@ -198,6 +200,11 @@ public StorageObjectSummaryCollection listObjects(String remoteStorageLocation, | |||
| logger.debug( | |||
| "Listing objects in the bucket {} with prefix {}", remoteStorageLocation, prefix); | |||
| Page<Blob> blobs = this.gcsClient.list(remoteStorageLocation, BlobListOption.prefix(prefix)); | |||
| // Normal flow will never hit here. This is only for testing purposes | |||
| if (isInjectedExceptionEnabled() | |||
| && SnowflakeGCSClient.injectedException instanceof StorageProviderException) { | |||
There was a problem hiding this comment.
I don't see why this instanceof is needed. If we inject an exception, then we shouldn't check it's type, just throw it. If you want to have a guarantee of it's type, just make the injected exception of this type instead of throwable
| public void testDiagnosticCheckFailsWithNoAllowlistFileProvided() throws SQLException { | ||
| Properties props = new Properties(); | ||
| props.put("ENABLE_DIAGNOSTICS", true); | ||
| try (Connection con = getConnection(props)) { |
There was a problem hiding this comment.
Use assertThrows instead
| Properties props = new Properties(); | ||
| props.put("ENABLE_DIAGNOSTICS", true); | ||
| props.put("DIAGNOSTICS_ALLOWLIST_FILE", "/some/path/allowlist.json"); | ||
| try (Connection con = getConnection(props)) { |
There was a problem hiding this comment.
Same here, assetThrows
| statement.execute("DROP STAGE if exists testStage"); | ||
| } | ||
| } | ||
| SnowflakeGCSClient.setInjectedException(null); |
There was a problem hiding this comment.
This should be in a finally block. If the DROP STAGE fails for whatever reason, this would not be called.
| @@ -1489,55 +1489,6 @@ public void testNoSpaceLeftOnDeviceException() throws SQLException { | |||
| } | |||
| } | |||
|
|
|||
| @Test | |||
| @Disabled // TODO: ignored until SNOW-1616480 is resolved | |||
| public void testUploadWithGCSPresignedUrlWithoutConnection() throws Throwable { | |||
There was a problem hiding this comment.
Why remove this test?
| // Normal flow will never hit here. This is only for testing purposes | ||
| if (isInjectedExceptionEnabled() | ||
| && SnowflakeGCSClient.injectedException instanceof StorageProviderException) { | ||
| throw (StorageProviderException) SnowflakeGCSClient.injectedException; |
There was a problem hiding this comment.
This exception should be a StorageException to accurately mimic a failure. See the documentation for the list(...) method.
| @@ -101,6 +101,8 @@ public class SnowflakeGCSClient implements SnowflakeStorageClient { | |||
|
|
|||
| private static final SFLogger logger = SFLoggerFactory.getLogger(SnowflakeGCSClient.class); | |||
|
|
|||
| private static Throwable injectedException = null; // for testing purpose | |||
There was a problem hiding this comment.
Did you consider using mocks instead for this test?
Overview
SNOW-1016470
Pre-review self checklist
masterbranchmvn -P check-style validate)mvn verifyand inspecttarget/japicmp/japicmp.html)SNOW-XXXX:External contributors - please answer these questions before submitting a pull request. Thanks!
What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Issue: #NNNN
Fill out the following pre-review checklist:
@SnowflakeJdbcInternalApi(note that public/protected methods/fields in classes marked with this annotation are already internal)Please describe how your code solves the related issue.
Adding tests and refactoring to increase code coverage