Conversation
|
Output: |
|
TODO: add integration test |
pietern
left a comment
pietern
left a comment
There was a problem hiding this comment.
Please also include the integration test for the filer code (there are existing filer tests).
| return fmt.Errorf("invalid output format: %s", c.outputFormat) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
You can set the "template" annotation on the command to achieve the same result. Then this function doesn't need to be added.
There was a problem hiding this comment.
tried that first, does not work because the cmdIO object is initialized in RootCmd, so overriding annotation does not work
There was a problem hiding this comment.
@shreyas-goenka please add this as a comment in the command and to this function.
Also, please make Render to call RenderWithTemplate with c.template, otherwise we have two code paths for the same thing
There was a problem hiding this comment.
done in followup
| } | ||
|
|
||
| // Get file info | ||
| filesInfo, err := f.ReadDir(ctx, "") |
There was a problem hiding this comment.
Well no right? since the filer is rooted at the arg[0] already
There was a problem hiding this comment.
Ack, you're right. Could you modify this to . to make it explicit, and include a comment saying so?
| output := make([]map[string]string, 0) | ||
| for _, info := range filesInfo { | ||
| output = append(output, parseFileInfo(info, path, absolute)) | ||
| } |
There was a problem hiding this comment.
You could alias the FileInfo struct and add a couple helper functions to avoid creating a map[string]string. Those helpers would then also be more easily testable.
pietern
left a comment
pietern
left a comment
There was a problem hiding this comment.
Please augment the filer integration test to cover the additional functions.
I notice the path.Base call there, implying the returned paths are relative to the specified path, not the filer root. This needs to be covered by an integration test as it is ambiguous otherwise.
There is overlap between the filer integration test and the new workspace testdata helpers. Can you consolidate this in a follow up PR? I'm not keen on the proliferation of duplicate test code.
|
|
||
| // Make API request | ||
| err = apiClient.Do(ctx, http.MethodPost, urlPath, content, nil) | ||
| require.NoError(w.t, err) |
There was a problem hiding this comment.
This is equivalent to what filer.Filer does. Please reuse that instead of a separate impl.
|
|
||
| func assertObjectListed(t *testing.T, parsedLogs []map[string]string, name string, objectType string) { | ||
| foundFile := false | ||
| for _, v := range parsedLogs { |
There was a problem hiding this comment.
How are these parsed logs? Looking at the code I see []filer.FileInfo.
| err := json.Unmarshal(stdout.Bytes(), &parsedLogs) | ||
| require.NoError(t, err) | ||
|
|
||
| // make assertions on the output logs |
There was a problem hiding this comment.
Please refer to just "output". The "logs" suffix confuses it with our logging infrastructure.
There was a problem hiding this comment.
+1, we need a couple of type-safe CLI run test helpers
This cherry-picks the filer changes from #408.
## Changes This cherry-picks the filer changes from #408. ## Tests Manually ran integration tests.
| `) | ||
| if longMode { | ||
| template = cmdio.Heredoc(` | ||
| {{range .}}{{.Type|printf "%-10s"}} {{.Size}} {{.ModTime|unix_date}} {{.Name}} |
There was a problem hiding this comment.
what if we support just the long mode? then the code is more maintainable and template ends up in the annotation
| return fmt.Errorf("invalid output format: %s", c.outputFormat) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
@shreyas-goenka please add this as a comment in the command and to this function.
Also, please make Render to call RenderWithTemplate with c.template, otherwise we have two code paths for the same thing
| // Initialize workspace client | ||
| ctx := cmd.Context() | ||
| w := root.WorkspaceClient(ctx) | ||
| f, err := filer.NewWorkspaceFilesClient(w, rootPath) |
There was a problem hiding this comment.
databricks fs lists DBFS, not the workspace. why do we use workspace files?
| err := json.Unmarshal(stdout.Bytes(), &parsedLogs) | ||
| require.NoError(t, err) | ||
|
|
||
| // make assertions on the output logs |
There was a problem hiding this comment.
+1, we need a couple of type-safe CLI run test helpers
| return w.root | ||
| } | ||
|
|
||
| func (w *workspaceTestdata) AddFile(name string, content string) { |
There was a problem hiding this comment.
can you also add a test for DBFS?
| return string(b), nil | ||
| }, | ||
| "unix_date": func(t time.Time) string { | ||
| return t.UTC().Format(time.UnixDate) |
There was a problem hiding this comment.
i've looked at the output - it's very difficult to parse when piped through aws. use the 2006-01-02T15:04:05Z as a format.
There was a problem hiding this comment.
done in the followup PR
There was a problem hiding this comment.
done in the followup PR
|
This PR is continued in #429. |
4 participants
Tests
Manually