damondouglas commented on code in PR #24874:
URL: https://github.com/apache/beam/pull/24874#discussion_r1082831517
##########
playground/backend/internal/setup_tools/builder/setup_builder.go:
##########
@@ -185,7 +196,7 @@ func replaceLogPlaceholder(paths *fs_tool.LifeCyclePaths,
executorConfig *enviro
}
// GetFirstFileFromFolder return a name of the first file in a specified folder
-func GetFirstFileFromFolder(folderAbsolutePath string) string {
- files, _ := filepath.Glob(fmt.Sprintf("%s/*%s", folderAbsolutePath,
fs_tool.JavaSourceFileExtension))
- return files[0]
+func GetFilesFromFolder(folderAbsolutePath string, extension string) []string {
+ files, _ := filepath.Glob(fmt.Sprintf("%s/*%s", folderAbsolutePath,
extension))
Review Comment:
What if `filepath.Glob` returns an error?
##########
playground/backend/internal/fs_tool/java_fs_test.go:
##########
@@ -144,27 +171,78 @@ func Test_executableName(t *testing.T) {
name: "Multiple files where one of them is main",
prepare: func() {
compiled := filepath.Join(workDir,
pipelinesFolder, pipelineId.String(), compiledFolderName)
- secondaryFilePath := filepath.Join(compiled,
"temp.scala")
- err := os.WriteFile(secondaryFilePath,
[]byte("TEMP_DATA"), 0600)
+ primaryFilePath := filepath.Join(compiled,
"main.scala")
+ err := os.WriteFile(primaryFilePath,
[]byte("object MinimalWordCount {def main(cmdlineArgs: Array[String]): Unit =
{}}"), 0600)
if err != nil {
panic(err)
}
- primaryFilePath := filepath.Join(compiled,
"main.scala")
- err = os.WriteFile(primaryFilePath,
[]byte("object MinimalWordCount {def main(cmdlineArgs: Array[String]): Unit =
{}}"), 0600)
+ secondaryFilePath := filepath.Join(compiled,
"temp.scala")
+ err = os.WriteFile(secondaryFilePath,
[]byte("TEMP_DATA"), 0600)
if err != nil {
panic(err)
}
},
+ cleanup: cleanupFunc,
args: args{
executableFolder: filepath.Join(workDir,
pipelinesFolder, pipelineId.String(), "bin"),
},
want: "main",
wantErr: false,
},
+ {
+ // Test case with calling sourceFileName method with
multiple files where one of them is a .class file
+ // with main() method
+ // As a result, want to receive a name that should be
executed
+ name: "Multiple Java class files where one of them
contains main",
+ prepare: func() {
+ testdataPath := "java_testdata"
+ dirEntries, err := os.ReadDir(testdataPath)
+ if err != nil {
+ panic(err)
Review Comment:
Return error, and fail test as a result of the error.
##########
playground/backend/internal/fs_tool/java_fs_test.go:
##########
@@ -144,27 +171,78 @@ func Test_executableName(t *testing.T) {
name: "Multiple files where one of them is main",
prepare: func() {
compiled := filepath.Join(workDir,
pipelinesFolder, pipelineId.String(), compiledFolderName)
- secondaryFilePath := filepath.Join(compiled,
"temp.scala")
- err := os.WriteFile(secondaryFilePath,
[]byte("TEMP_DATA"), 0600)
+ primaryFilePath := filepath.Join(compiled,
"main.scala")
+ err := os.WriteFile(primaryFilePath,
[]byte("object MinimalWordCount {def main(cmdlineArgs: Array[String]): Unit =
{}}"), 0600)
if err != nil {
panic(err)
}
- primaryFilePath := filepath.Join(compiled,
"main.scala")
- err = os.WriteFile(primaryFilePath,
[]byte("object MinimalWordCount {def main(cmdlineArgs: Array[String]): Unit =
{}}"), 0600)
+ secondaryFilePath := filepath.Join(compiled,
"temp.scala")
+ err = os.WriteFile(secondaryFilePath,
[]byte("TEMP_DATA"), 0600)
if err != nil {
panic(err)
}
},
+ cleanup: cleanupFunc,
args: args{
executableFolder: filepath.Join(workDir,
pipelinesFolder, pipelineId.String(), "bin"),
},
want: "main",
wantErr: false,
},
+ {
+ // Test case with calling sourceFileName method with
multiple files where one of them is a .class file
+ // with main() method
+ // As a result, want to receive a name that should be
executed
+ name: "Multiple Java class files where one of them
contains main",
+ prepare: func() {
+ testdataPath := "java_testdata"
+ dirEntries, err := os.ReadDir(testdataPath)
+ if err != nil {
+ panic(err)
+ }
+
+ compiled := filepath.Join(workDir,
pipelinesFolder, pipelineId.String(), compiledFolderName)
+
+ for _, entry := range dirEntries {
+ src := filepath.Join(testdataPath,
entry.Name())
+ dst := filepath.Join(compiled,
entry.Name())
+ err = os.Link(src, dst)
+ if err != nil {
+ panic(err)
Review Comment:
Instead return error and fail test as a result of the error.
##########
playground/backend/internal/fs_tool/java_fs_test.go:
##########
@@ -85,15 +85,39 @@ func Test_executableName(t *testing.T) {
workDir := "workingDir"
preparedPipelinesFolder := filepath.Join(workDir, pipelinesFolder)
lc := newJavaLifeCycle(pipelineId, preparedPipelinesFolder)
- lc.CreateFolders()
- defer os.RemoveAll(workDir)
+ err := lc.CreateFolders()
+ if err != nil {
+ panic(err)
+ }
+ defer func() {
+ err := os.RemoveAll(workDir)
+ if err != nil {
+ panic(err)
+ }
+ }()
+
+ cleanupFunc := func() {
+ compiled := filepath.Join(workDir, pipelinesFolder,
pipelineId.String(), compiledFolderName)
+ dirEntries, err := os.ReadDir(compiled)
+ if err != nil {
+ panic(err)
+ }
+
+ for _, entry := range dirEntries {
+ err := os.Remove(filepath.Join(compiled, entry.Name()))
+ if err != nil {
+ panic(err)
+ }
+ }
+ }
type args struct {
executableFolder string
}
tests := []struct {
name string
prepare func()
+ cleanup func()
args args
want string
wantErr bool
Review Comment:
I didn't see any tests where `wantErr=true`
##########
playground/backend/internal/fs_tool/java_fs_test.go:
##########
@@ -85,15 +85,39 @@ func Test_executableName(t *testing.T) {
workDir := "workingDir"
preparedPipelinesFolder := filepath.Join(workDir, pipelinesFolder)
lc := newJavaLifeCycle(pipelineId, preparedPipelinesFolder)
- lc.CreateFolders()
- defer os.RemoveAll(workDir)
+ err := lc.CreateFolders()
+ if err != nil {
+ panic(err)
+ }
+ defer func() {
+ err := os.RemoveAll(workDir)
+ if err != nil {
+ panic(err)
+ }
+ }()
+
+ cleanupFunc := func() {
+ compiled := filepath.Join(workDir, pipelinesFolder,
pipelineId.String(), compiledFolderName)
+ dirEntries, err := os.ReadDir(compiled)
+ if err != nil {
+ panic(err)
Review Comment:
Return error, and fail test as a result of the error.
##########
playground/backend/internal/fs_tool/java_fs_test.go:
##########
@@ -85,15 +85,39 @@ func Test_executableName(t *testing.T) {
workDir := "workingDir"
preparedPipelinesFolder := filepath.Join(workDir, pipelinesFolder)
lc := newJavaLifeCycle(pipelineId, preparedPipelinesFolder)
- lc.CreateFolders()
- defer os.RemoveAll(workDir)
+ err := lc.CreateFolders()
+ if err != nil {
+ panic(err)
+ }
+ defer func() {
+ err := os.RemoveAll(workDir)
+ if err != nil {
+ panic(err)
+ }
+ }()
+
+ cleanupFunc := func() {
+ compiled := filepath.Join(workDir, pipelinesFolder,
pipelineId.String(), compiledFolderName)
+ dirEntries, err := os.ReadDir(compiled)
+ if err != nil {
+ panic(err)
+ }
+
+ for _, entry := range dirEntries {
+ err := os.Remove(filepath.Join(compiled, entry.Name()))
+ if err != nil {
+ panic(err)
Review Comment:
Return error, and fail test as a result of the error.
##########
playground/backend/internal/fs_tool/java_fs_test.go:
##########
@@ -85,15 +85,39 @@ func Test_executableName(t *testing.T) {
workDir := "workingDir"
preparedPipelinesFolder := filepath.Join(workDir, pipelinesFolder)
lc := newJavaLifeCycle(pipelineId, preparedPipelinesFolder)
- lc.CreateFolders()
- defer os.RemoveAll(workDir)
+ err := lc.CreateFolders()
+ if err != nil {
+ panic(err)
+ }
+ defer func() {
+ err := os.RemoveAll(workDir)
+ if err != nil {
+ panic(err)
Review Comment:
Return error, and fail test as a result of the error.
##########
playground/backend/internal/executors/executor_builder.go:
##########
@@ -82,87 +82,97 @@ func (b *ExecutorBuilder) WithTestRunner()
*UnitTestExecutorBuilder {
return &UnitTestExecutorBuilder{*b}
}
-//WithCommand adds compile command to executor
+// WithCommand adds compile command to executor
func (b *CompileBuilder) WithCommand(compileCmd string) *CompileBuilder {
b.actions = append(b.actions, func(e *Executor) {
e.compileArgs.commandName = compileCmd
})
return b
}
-//WithWorkingDir adds dir path to executor
+// WithWorkingDir adds dir path to executor
func (b *CompileBuilder) WithWorkingDir(dir string) *CompileBuilder {
b.actions = append(b.actions, func(e *Executor) {
e.compileArgs.workingDir = dir
})
return b
}
-//WithArgs adds compile args to executor
+// WithArgs adds compile args to executor
func (b *CompileBuilder) WithArgs(compileArgs []string) *CompileBuilder {
b.actions = append(b.actions, func(e *Executor) {
e.compileArgs.commandArgs = compileArgs
})
return b
}
-//WithFileName adds file name to executor
+// WithFileName adds file name to executor
func (b *CompileBuilder) WithFileName(fileName string) *CompileBuilder {
+ return b.WithFileNames([]string{fileName})
+}
+
+// WithFileNames adds file names to executor
+func (b *CompileBuilder) WithFileNames(fileNames []string) *CompileBuilder {
b.actions = append(b.actions, func(e *Executor) {
- e.compileArgs.fileName = fileName
+ e.compileArgs.fileNames = fileNames
})
return b
}
-//WithExecutableFileName adds file name to executor
+// WithExecutableFileName adds file name to executor
func (b *RunBuilder) WithExecutableFileName(name string) *RunBuilder {
+ return b.WithExecutableFileNames([]string{name})
+}
+
+// WithExecutableFileNames adds file name to executor
+func (b *RunBuilder) WithExecutableFileNames(names []string) *RunBuilder {
b.actions = append(b.actions, func(e *Executor) {
- e.runArgs.fileName = name
+ e.runArgs.fileNames = names
})
return b
}
-//WithWorkingDir adds dir path to executor
+// WithWorkingDir adds dir path to executor
func (b *RunBuilder) WithWorkingDir(dir string) *RunBuilder {
b.actions = append(b.actions, func(e *Executor) {
e.runArgs.workingDir = dir
})
return b
}
-//WithCommand adds run command to executor
+// WithCommand adds run command to executor
func (b *RunBuilder) WithCommand(runCmd string) *RunBuilder {
b.actions = append(b.actions, func(e *Executor) {
e.runArgs.commandName = runCmd
})
return b
}
-//WithArgs adds run args to executor
+// WithArgs adds run args to executor
func (b *RunBuilder) WithArgs(runArgs []string) *RunBuilder {
b.actions = append(b.actions, func(e *Executor) {
e.runArgs.commandArgs = runArgs
})
return b
}
-//WithGraphOutput adds the need of graph output to executor
+// WithGraphOutput adds the need of graph output to executor
func (b *RunBuilder) WithGraphOutput() *RunBuilder {
b.actions = append(b.actions, func(e *Executor) {
//todo
Review Comment:
What is the work of the `//todo`? Could we instead refer to a GitHub issue?
##########
playground/backend/internal/fs_tool/java_fs_test.go:
##########
@@ -85,15 +85,39 @@ func Test_executableName(t *testing.T) {
workDir := "workingDir"
preparedPipelinesFolder := filepath.Join(workDir, pipelinesFolder)
lc := newJavaLifeCycle(pipelineId, preparedPipelinesFolder)
- lc.CreateFolders()
- defer os.RemoveAll(workDir)
+ err := lc.CreateFolders()
+ if err != nil {
+ panic(err)
Review Comment:
Return error, and fail test as a result of the error.
##########
playground/backend/internal/executors/executor_builder.go:
##########
@@ -82,87 +82,97 @@ func (b *ExecutorBuilder) WithTestRunner()
*UnitTestExecutorBuilder {
return &UnitTestExecutorBuilder{*b}
}
-//WithCommand adds compile command to executor
+// WithCommand adds compile command to executor
func (b *CompileBuilder) WithCommand(compileCmd string) *CompileBuilder {
b.actions = append(b.actions, func(e *Executor) {
e.compileArgs.commandName = compileCmd
})
return b
}
-//WithWorkingDir adds dir path to executor
+// WithWorkingDir adds dir path to executor
func (b *CompileBuilder) WithWorkingDir(dir string) *CompileBuilder {
b.actions = append(b.actions, func(e *Executor) {
e.compileArgs.workingDir = dir
})
return b
}
-//WithArgs adds compile args to executor
+// WithArgs adds compile args to executor
func (b *CompileBuilder) WithArgs(compileArgs []string) *CompileBuilder {
b.actions = append(b.actions, func(e *Executor) {
e.compileArgs.commandArgs = compileArgs
})
return b
}
-//WithFileName adds file name to executor
+// WithFileName adds file name to executor
func (b *CompileBuilder) WithFileName(fileName string) *CompileBuilder {
+ return b.WithFileNames([]string{fileName})
+}
+
+// WithFileNames adds file names to executor
+func (b *CompileBuilder) WithFileNames(fileNames []string) *CompileBuilder {
b.actions = append(b.actions, func(e *Executor) {
- e.compileArgs.fileName = fileName
+ e.compileArgs.fileNames = fileNames
})
return b
}
Review Comment:
What about just one func that handles both cases?:
```
func (e *CompileBuilder) WithFileNames(fileNames ...string) *CompileBuilder {
e.compileArgs.fileNames = fileNames
return e
}
```
https://go.dev/play/p/IQ59C-t8wNA
##########
playground/backend/internal/fs_tool/java_fs_test.go:
##########
@@ -85,15 +85,39 @@ func Test_executableName(t *testing.T) {
workDir := "workingDir"
preparedPipelinesFolder := filepath.Join(workDir, pipelinesFolder)
lc := newJavaLifeCycle(pipelineId, preparedPipelinesFolder)
- lc.CreateFolders()
- defer os.RemoveAll(workDir)
+ err := lc.CreateFolders()
+ if err != nil {
+ panic(err)
+ }
+ defer func() {
+ err := os.RemoveAll(workDir)
+ if err != nil {
+ panic(err)
+ }
+ }()
+
+ cleanupFunc := func() {
+ compiled := filepath.Join(workDir, pipelinesFolder,
pipelineId.String(), compiledFolderName)
+ dirEntries, err := os.ReadDir(compiled)
+ if err != nil {
+ panic(err)
+ }
+
+ for _, entry := range dirEntries {
+ err := os.Remove(filepath.Join(compiled, entry.Name()))
+ if err != nil {
+ panic(err)
Review Comment:
Return error, fail test with error.
##########
playground/backend/internal/executors/executor_builder.go:
##########
@@ -82,87 +82,97 @@ func (b *ExecutorBuilder) WithTestRunner()
*UnitTestExecutorBuilder {
return &UnitTestExecutorBuilder{*b}
}
-//WithCommand adds compile command to executor
+// WithCommand adds compile command to executor
func (b *CompileBuilder) WithCommand(compileCmd string) *CompileBuilder {
b.actions = append(b.actions, func(e *Executor) {
e.compileArgs.commandName = compileCmd
})
return b
}
-//WithWorkingDir adds dir path to executor
+// WithWorkingDir adds dir path to executor
func (b *CompileBuilder) WithWorkingDir(dir string) *CompileBuilder {
b.actions = append(b.actions, func(e *Executor) {
e.compileArgs.workingDir = dir
})
return b
}
-//WithArgs adds compile args to executor
+// WithArgs adds compile args to executor
func (b *CompileBuilder) WithArgs(compileArgs []string) *CompileBuilder {
b.actions = append(b.actions, func(e *Executor) {
e.compileArgs.commandArgs = compileArgs
})
return b
}
-//WithFileName adds file name to executor
+// WithFileName adds file name to executor
func (b *CompileBuilder) WithFileName(fileName string) *CompileBuilder {
+ return b.WithFileNames([]string{fileName})
+}
+
+// WithFileNames adds file names to executor
+func (b *CompileBuilder) WithFileNames(fileNames []string) *CompileBuilder {
b.actions = append(b.actions, func(e *Executor) {
- e.compileArgs.fileName = fileName
+ e.compileArgs.fileNames = fileNames
})
return b
}
-//WithExecutableFileName adds file name to executor
+// WithExecutableFileName adds file name to executor
func (b *RunBuilder) WithExecutableFileName(name string) *RunBuilder {
+ return b.WithExecutableFileNames([]string{name})
+}
+
+// WithExecutableFileNames adds file name to executor
+func (b *RunBuilder) WithExecutableFileNames(names []string) *RunBuilder {
b.actions = append(b.actions, func(e *Executor) {
- e.runArgs.fileName = name
+ e.runArgs.fileNames = names
})
return b
}
Review Comment:
Same as above comment `WithExecutableFileNames(names ...string) *RunBuilder`
##########
playground/backend/internal/fs_tool/java_fs_test.go:
##########
@@ -85,15 +85,39 @@ func Test_executableName(t *testing.T) {
workDir := "workingDir"
preparedPipelinesFolder := filepath.Join(workDir, pipelinesFolder)
lc := newJavaLifeCycle(pipelineId, preparedPipelinesFolder)
- lc.CreateFolders()
- defer os.RemoveAll(workDir)
+ err := lc.CreateFolders()
+ if err != nil {
+ panic(err)
+ }
+ defer func() {
+ err := os.RemoveAll(workDir)
+ if err != nil {
+ panic(err)
+ }
+ }()
+
+ cleanupFunc := func() {
+ compiled := filepath.Join(workDir, pipelinesFolder,
pipelineId.String(), compiledFolderName)
+ dirEntries, err := os.ReadDir(compiled)
+ if err != nil {
+ panic(err)
Review Comment:
Return error, fail test with error.
##########
playground/backend/internal/fs_tool/java_fs_test.go:
##########
@@ -85,15 +85,39 @@ func Test_executableName(t *testing.T) {
workDir := "workingDir"
preparedPipelinesFolder := filepath.Join(workDir, pipelinesFolder)
lc := newJavaLifeCycle(pipelineId, preparedPipelinesFolder)
- lc.CreateFolders()
- defer os.RemoveAll(workDir)
+ err := lc.CreateFolders()
+ if err != nil {
+ panic(err)
+ }
+ defer func() {
+ err := os.RemoveAll(workDir)
+ if err != nil {
+ panic(err)
Review Comment:
Return error, fail test with error.
##########
playground/backend/internal/fs_tool/java_testdata/Foo.java:
##########
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+class Bar {
+ public static void bar() {
+ System.out.println("Hello");
+ }
+}
+
+public class Foo {
+ public static void main(java.lang.String[] args) {
+ Bar.bar();
+ }
Review Comment:
Do we have a case that doesn't have main? Or it has main but no `String[]
args`? What about a Java source that has syntax or possible compile errors?
Additionally what about a source that has runtime errors?
##########
playground/backend/internal/executors/executor_builder.go:
##########
@@ -178,47 +188,47 @@ func (b *UnitTestExecutorBuilder) WithWorkingDir(dir
string) *UnitTestExecutorBu
return b
}
-//WithGraphOutput adds the need of graph output to executor
+// WithGraphOutput adds the need of graph output to executor
func (b *UnitTestExecutorBuilder) WithGraphOutput() *UnitTestExecutorBuilder {
b.actions = append(b.actions, func(e *Executor) {
//todo
Review Comment:
What is the work remaining of this `//todo`? Should it refer instead to a
GitHub issue?
##########
playground/backend/internal/fs_tool/java_fs_test.go:
##########
@@ -85,15 +85,39 @@ func Test_executableName(t *testing.T) {
workDir := "workingDir"
preparedPipelinesFolder := filepath.Join(workDir, pipelinesFolder)
lc := newJavaLifeCycle(pipelineId, preparedPipelinesFolder)
- lc.CreateFolders()
- defer os.RemoveAll(workDir)
+ err := lc.CreateFolders()
+ if err != nil {
+ panic(err)
Review Comment:
Return error, fail test with error.
--
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]
