Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(install): Refactor re.setOutput to use bufio scanner #1449

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 28 additions & 20 deletions internal/install/execution/go_task_recipe_executor.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package execution

import (
"bufio"
"bytes"
"context"
"encoding/json"
Expand Down Expand Up @@ -184,33 +185,40 @@
func (re *GoTaskRecipeExecutor) setOutput(outputFileName string) {
outputFile, err := os.Open(outputFileName)
if err != nil {
log.Debugf("error openning json output file %s", outputFileName)
log.Debugf("error opening JSON output file %s", outputFileName)

Check warning on line 188 in internal/install/execution/go_task_recipe_executor.go

View check run for this annotation

Codecov / codecov/patch

internal/install/execution/go_task_recipe_executor.go#L188

Added line #L188 was not covered by tests
return
}
defer outputFile.Close()

outputBytes, _ := ioutil.ReadAll(outputFile)
if len(outputBytes) > 0 {
var result map[string]interface{}
lines := strings.Split(string(outputBytes), "\n")
for _, line := range lines {
if json.Valid([]byte(line)) {
var jsonFromString map[string]interface{}
if err := json.Unmarshal([]byte(line), &jsonFromString); err == nil {
if mergoErr := mergo.Merge(&result, jsonFromString, mergo.WithOverride); mergoErr != nil {
log.Debugf("error while merging JSON output resuls, details:%s", mergoErr.Error())
}
} else {
log.Debugf("error while unmarshaling JSON output, details:%s", err.Error())
}
} else {
log.Debugf(fmt.Sprintf("Invalid JSON string found in output file, skipping: %v", line))
output := map[string]interface{}{}
s := bufio.NewScanner(outputFile)
for s.Scan() {
bs := s.Bytes()
if json.Valid(bs) {
if err = mergeStringJSON(&output, bs); err != nil {
log.Debug(err.Error())

Check warning on line 199 in internal/install/execution/go_task_recipe_executor.go

View check run for this annotation

Codecov / codecov/patch

internal/install/execution/go_task_recipe_executor.go#L199

Added line #L199 was not covered by tests
}
} else {
log.Debugf("Invalid JSON string found in output file, skipping: %s", s.Text())
}
re.Output = NewOutputParser(result)
} else {
re.Output = NewOutputParser(map[string]interface{}{})
}
if err = s.Err(); err != nil {
log.Debugf("error scanning output file %s: %s", outputFileName, err.Error())
}

Check warning on line 207 in internal/install/execution/go_task_recipe_executor.go

View check run for this annotation

Codecov / codecov/patch

internal/install/execution/go_task_recipe_executor.go#L206-L207

Added lines #L206 - L207 were not covered by tests

re.Output = NewOutputParser(output)
}

func mergeStringJSON(output *map[string]interface{}, bs []byte) error {
var jsonFromString map[string]interface{}
var err error
if err = json.Unmarshal(bs, &jsonFromString); err != nil {
return fmt.Errorf("error unmarshaling JSON output: %w", err)
}
if err = mergo.Merge(output, jsonFromString, mergo.WithOverride); err != nil {
return fmt.Errorf("error merging JSON output: %w", err)
}

Check warning on line 220 in internal/install/execution/go_task_recipe_executor.go

View check run for this annotation

Codecov / codecov/patch

internal/install/execution/go_task_recipe_executor.go#L219-L220

Added lines #L219 - L220 were not covered by tests
return nil
}

func (re *GoTaskRecipeExecutor) GetOutput() *OutputParser {
Expand Down
68 changes: 65 additions & 3 deletions internal/install/execution/go_task_recipe_executor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,9 @@ import (
"strings"
"testing"

"github.com/newrelic/newrelic-cli/internal/install/types"
"github.com/stretchr/testify/assert"

"github.com/stretchr/testify/require"

"github.com/newrelic/newrelic-cli/internal/install/types"
)

func TestExecute_SystemVariableInterpolation(t *testing.T) {
Expand Down Expand Up @@ -486,3 +484,67 @@ tasks:
assert.Empty(t, e.GetOutput().Metadata()["something"])
assert.Equal(t, "very", e.GetOutput().Metadata()["clean"])
}

func Test_mergeStringJSON(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a long test, perhaps we could break it into many small tests?

type args struct {
output *map[string]interface{}
bs []byte
}
outputNewMap := map[string]interface{}{}
argsNewMap := args{
output: &outputNewMap,
bs: []byte("{\"Metadata\":{\"some\":\"thing\"}}\n"),
}
outputExistingMap := map[string]interface{}{
"Metadata": map[string]interface{}{"some": "thing"},
}
argsExistingMap := args{
output: &outputExistingMap,
bs: []byte("{\"Metadata\":{\"another\":\"thing\"}}\n"),
}
outputInvalidJSON := map[string]interface{}{}
argsInvalidJSON := args{
output: &outputInvalidJSON,
bs: []byte("Invalid JSON"),
}
outputEmptyString := map[string]interface{}{}
argsEmptyString := args{
output: &outputEmptyString,
bs: []byte(""),
}
tests := []struct {
name string
args args
wantErr bool
wantLen int
}{
{"new map", argsNewMap, false, 1},
{"existing map", argsExistingMap, false, 2},
{"invalid JSON", argsInvalidJSON, true, 0},
{"empty string", argsEmptyString, true, 0},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if err := mergeStringJSON(tt.args.output, tt.args.bs); (err != nil) != tt.wantErr {
t.Errorf("mergeStringJSON() error = %v, wantErr %v", err, tt.wantErr)
}
output := *tt.args.output
if output == nil {
t.Errorf("expected output map, got nil")
}
if tt.wantLen == 0 {
if len(output) > 0 {
t.Errorf("unexpected number of results -- expected 0, got %d", len(output))
}
} else {
metadata := output["Metadata"].(map[string]interface{})
if metadata == nil {
t.Errorf("expected 'Metadata' field in output map, got nil")
}
if len(metadata) != tt.wantLen {
t.Errorf("unexpected number of results -- expected %d, got %d\n results: %v", tt.wantLen, len(metadata), tt.args.output)
}
}
})
}
}