Fix existing local file integration tests for streaming writes flow. #2902
Conversation
meet2mky
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2902 +/- ##
==========================================
- Coverage 76.12% 76.09% -0.04%
==========================================
Files 118 118
Lines 16659 16659
==========================================
- Hits 12681 12676 -5
- Misses 3440 3444 +4
- Partials 538 539 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
meet2mky
force-pushed
the
streaming-writes-local-file-it
branch
from
ce0f34b to
289abe5
Compare
meet2mky
force-pushed
the
streaming-writes-local-file-it
branch
from
289abe5 to
24d0d7e
Compare
| @@ -103,6 +105,19 @@ func LogFile() string { | |||
| return logFile | |||
| } | |||
|
|
|||
| func SetStreamingWritesEnabled(flags []string) { | |||
There was a problem hiding this comment.
I would like to take @ashmeenkaur opinion on how to refactor local_file tests to run for streaming_writes=true scenario.
We have done it this way for streaming_writes package:https://github.com/GoogleCloudPlatform/gcsfuse/blob/master/tools/integration_tests/streaming_writes/default_mount_local_file_test.go
Probably here its too much of work to refactor, lets take a call with Ashmeen
There was a problem hiding this comment.
Edit: Thinking a bit more about it, I think it makes sense to create a common suite and run all these tests for local file without streaming files, local file with streaming writes and empty gcs object with streaming writes since there will be considerable overlap in the 3. That way, instead of having if-else statements in the test, we can have separate tests in the respective test suites for scenarios which differ:
eg:
if setup.StreamingWritesEnabled() {
// First out of order write ensures the existing sequentially written data is uploaded
// to GCS when streaming writes are enabled.
ValidateObjectContentsFromGCS(ctx, storageClient, testDirName, FileName1, "string1", t)
} else {
ValidateObjectNotFoundErrOnGCS(ctx, storageClient, testDirName, FileName1, t)
}
```
| @@ -98,7 +98,9 @@ func TestMain(m *testing.M) { | |||
| // Not setting config file explicitly with 'create-empty-file: false' as it is default. | |||
| flagsSet := [][]string{ | |||
| {"--implicit-dirs=true", "--rename-dir-limit=3"}, | |||
| {"--implicit-dirs=false", "--rename-dir-limit=3"}} | |||
| {"--implicit-dirs=false", "--rename-dir-limit=3"}, | |||
| {"--enable-streaming-writes=true", "--write-block-size-mb=2", "--write-max-blocks-per-file=2"}, | |||
There was a problem hiding this comment.
shall we combine this with implicit-dirs false tests in favor of saving some test execution time.
There was a problem hiding this comment.
You mean if we add "--implicit-dirs=false" flag to the mount then it makes the mounting faster because it doesn't have to list recursive directories? Or is there other reason for fast test execution in this case?
There was a problem hiding this comment.
No I meant instead of having 3 flagsets to run the tests with, we can just have 2 by combining implicit dirs false(default) with streaming writes tests
There was a problem hiding this comment.
Okay got it we keep less flagsets combination.
Description
Fixing existing local file tests for streaming writes flow. Mounts with streaming writes enabled do not support read operation and truncate operation (truncate to lower size than current file size).
Link to the issue in case of a bug fix.
b/388722649
Testing details