Skip to content

Commit 7df05c2

Browse files
Electronic-WasteElectronic-Waste
authored
fix: clean up UTs for file metrics collector (#2285)
* chore: replace testDir with tempDir. Signed-off-by: Electronic-Waste <[email protected]> * fix: expose and compare errors. Signed-off-by: Electronic-Waste <[email protected]> * refactor: integrate test generation func into testCases. Signed-off-by: Electronic-Waste <[email protected]> * refactor: update error comparing mechanism. Signed-off-by: Electronic-Waste <[email protected]> * fix: make some changes under the review of yuki. Signed-off-by: Electronic-Waste <[email protected]> --------- Signed-off-by: Electronic-Waste <[email protected]>
1 parent 9680b8c commit 7df05c2

File tree

2 files changed

+77
-166
lines changed

2 files changed

+77
-166
lines changed

‎pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector.go‎

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package sidecarmetricscollector
1818

1919
import (
2020
"encoding/json"
21+
"errors"
2122
"fmt"
2223
"io"
2324
"os"
@@ -34,15 +35,27 @@ import (
3435
"github.com/kubeflow/katib/pkg/metricscollector/v1beta1/common"
3536
)
3637

38+
var (
39+
errFileFormat = fmt.Errorf("format must be set %v or %v", commonv1beta1.TextFormat, commonv1beta1.JsonFormat)
40+
errOpenFile = errors.New("failed to open the file")
41+
errReadFile = errors.New("failed to read the file")
42+
errParseJson = errors.New("failed to parse the json object")
43+
)
44+
3745
func CollectObservationLog(fileName string, metrics []string, filters []string, fileFormat commonv1beta1.FileFormat) (*v1beta1.ObservationLog, error) {
46+
// we should check fileFormat first in case of opening an invalid file
47+
if fileFormat != commonv1beta1.JsonFormat && fileFormat != commonv1beta1.TextFormat {
48+
return nil, errFileFormat
49+
}
50+
3851
file, err := os.Open(fileName)
3952
if err != nil {
40-
return nil, err
53+
return nil, fmt.Errorf("%w: %s", errOpenFile, err.Error())
4154
}
4255
defer file.Close()
4356
content, err := io.ReadAll(file)
4457
if err != nil {
45-
return nil, err
58+
return nil, fmt.Errorf("%w: %s", errReadFile, err.Error())
4659
}
4760
logs := string(content)
4861

@@ -51,8 +64,9 @@ func CollectObservationLog(fileName string, metrics []string, filters []string,
5164
return parseLogsInTextFormat(strings.Split(logs, "\n"), metrics, filters)
5265
case commonv1beta1.JsonFormat:
5366
return parseLogsInJsonFormat(strings.Split(logs, "\n"), metrics)
67+
default:
68+
return nil, nil
5469
}
55-
return nil, fmt.Errorf("format must be set %v or %v", commonv1beta1.TextFormat, commonv1beta1.JsonFormat)
5670
}
5771

5872
func parseLogsInTextFormat(logs []string, metrics []string, filters []string) (*v1beta1.ObservationLog, error) {
@@ -120,7 +134,7 @@ func parseLogsInJsonFormat(logs []string, metrics []string) (*v1beta1.Observatio
120134
}
121135
var jsonObj map[string]interface{}
122136
if err := json.Unmarshal([]byte(logline), &jsonObj); err != nil {
123-
return nil, err
137+
return nil, fmt.Errorf("%w: %s", errParseJson, err.Error())
124138
}
125139

126140
timestamp := time.Time{}.UTC().Format(time.RFC3339)

‎pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector_test.go‎

Lines changed: 59 additions & 162 deletions
Original file line numberDiff line numberDiff line change
@@ -23,52 +23,29 @@ import (
2323
"time"
2424

2525
"github.com/google/go-cmp/cmp"
26+
"github.com/google/go-cmp/cmp/cmpopts"
2627
commonv1beta1 "github.com/kubeflow/katib/pkg/apis/controller/common/v1beta1"
2728
v1beta1 "github.com/kubeflow/katib/pkg/apis/manager/v1beta1"
2829
"github.com/kubeflow/katib/pkg/controller.v1beta1/consts"
2930
)
3031

31-
const (
32-
validJSONTestFile = "good.json"
33-
invalidFormatJSONTestFile = "invalid-format.json"
34-
invalidTimestampJSONTestFile = "invalid-timestamp.json"
35-
missingMetricJSONTestFile = "missing-objective-metric.json"
36-
37-
validTEXTTestFile = "good.log"
38-
invalidValueTEXTTestFile = "invalid-value.log"
39-
invalidFormatTEXTTestFile = "invalid-format.log"
40-
invalidTimestampTEXTTestFile = "invalid-timestamp.log"
41-
missingMetricTEXTTestFile = "missing-objective-metric.log"
42-
)
43-
44-
var (
45-
testDir = "testdata"
46-
testJsonDataPath = filepath.Join(testDir, "JSON")
47-
testTextDataPath = filepath.Join(testDir, "TEXT")
48-
)
49-
5032
func TestCollectObservationLog(t *testing.T) {
51-
if err := generateTestDirs(); err != nil {
52-
t.Fatal(err)
53-
}
54-
if err := generateJSONTestFiles(); err != nil {
55-
t.Fatal(err)
56-
}
57-
if err := generateTEXTTestFiles(); err != nil {
58-
t.Fatal(err)
59-
}
60-
defer os.RemoveAll(testDir)
61-
6233
testCases := map[string]struct {
63-
filePath string
34+
fileName string
35+
testData string
6436
metrics []string
6537
filters []string
6638
fileFormat commonv1beta1.FileFormat
67-
err bool
39+
wantError error
6840
expected *v1beta1.ObservationLog
6941
}{
7042
"Positive case for logs in JSON format": {
71-
filePath: filepath.Join(testJsonDataPath, validJSONTestFile),
43+
fileName: "good.json",
44+
testData: `{"checkpoint_path": "", "global_step": "0", "loss": "0.22082142531871796", "timestamp": 1638422847.28721, "trial": "0"}
45+
{"acc": "0.9349666833877563", "checkpoint_path": "", "global_step": "0", "timestamp": 1638422847.287801, "trial": "0"}
46+
{"checkpoint_path": "", "global_step": "1", "loss": "0.1414974331855774", "timestamp": "2021-12-02T14:27:50.000035161Z", "trial": "0"}
47+
{"acc": "0.9586416482925415", "checkpoint_path": "", "global_step": "1", "timestamp": "2021-12-02T14:27:50.000037459Z", "trial": "0"}
48+
{"checkpoint_path": "", "global_step": "2", "loss": "0.10683439671993256", "trial": "0"}`,
7249
metrics: []string{"acc", "loss"},
7350
fileFormat: commonv1beta1.JsonFormat,
7451
expected: &v1beta1.ObservationLog{
@@ -112,7 +89,14 @@ func TestCollectObservationLog(t *testing.T) {
11289
},
11390
},
11491
"Positive case for logs in TEXT format": {
115-
filePath: filepath.Join(testTextDataPath, validTEXTTestFile),
92+
fileName: "good.log",
93+
testData: `2024-03-04T17:55:08Z INFO {metricName: accuracy, metricValue: 0.8078};{metricName: loss, metricValue: 0.5183}
94+
2024-03-04T17:55:08Z INFO {metricName: accuracy, metricValue: 0.6752}
95+
2024-03-04T17:55:08Z INFO {metricName: loss, metricValue: 0.3634}
96+
2024-03-04T17:55:08Z INFO {metricName: accuracy, metricValue: 100}
97+
2024-03-04T17:55:08Z INFO {metricName: accuracy, metricValue: 888.333}
98+
2024-03-04T17:55:08Z INFO {metricName: accuracy, metricValue: -0.4759}
99+
{metricName: loss, metricValue: 0.8671}`,
116100
metrics: []string{"accuracy", "loss"},
117101
filters: []string{"{metricName: ([\\w|-]+), metricValue: ((-?\\d+)(\\.\\d+)?)}"},
118102
fileFormat: commonv1beta1.TextFormat,
@@ -178,7 +162,11 @@ func TestCollectObservationLog(t *testing.T) {
178162
},
179163
},
180164
"Invalid case for logs in TEXT format": {
181-
filePath: filepath.Join(testTextDataPath, invalidValueTEXTTestFile),
165+
fileName: "invalid-value.log",
166+
testData: `2024-03-04T17:55:08Z INFO {metricName: accuracy, metricValue: .333}
167+
2024-03-04T17:55:08Z INFO {metricName: accuracy, metricValue: -.333}
168+
2024-03-04T17:55:08Z INFO {metricName: accuracy, metricValue: - 345.333}
169+
2024-03-04T17:55:08Z INFO {metricName: accuracy, metricValue: 888.}`,
182170
filters: []string{"{metricName: ([\\w|-]+), metricValue: ((-?\\d+)(\\.\\d+)?)}"},
183171
metrics: []string{"accuracy", "loss"},
184172
fileFormat: commonv1beta1.TextFormat,
@@ -195,21 +183,26 @@ func TestCollectObservationLog(t *testing.T) {
195183
},
196184
},
197185
"Invalid file name": {
198-
filePath: "invalid",
199-
err: true,
186+
fileName: "invalid",
187+
fileFormat: commonv1beta1.JsonFormat,
188+
wantError: errOpenFile,
200189
},
201190
"Invalid file format": {
202-
filePath: filepath.Join(testTextDataPath, validTEXTTestFile),
191+
fileName: "good.log",
203192
fileFormat: "invalid",
204-
err: true,
193+
wantError: errFileFormat,
205194
},
206195
"Invalid formatted file for logs in JSON format": {
207-
filePath: filepath.Join(testJsonDataPath, invalidFormatJSONTestFile),
196+
fileName: "invalid-format.json",
197+
testData: `"checkpoint_path": "", "global_step": "0", "loss": "0.22082142531871796", "timestamp": 1638422847.28721, "trial": "0"
198+
{"acc": "0.9349666833877563", "checkpoint_path": "", "global_step": "0", "timestamp": 1638422847.287801, "trial": "0`,
208199
fileFormat: commonv1beta1.JsonFormat,
209-
err: true,
200+
wantError: errParseJson,
210201
},
211202
"Invalid formatted file for logs in TEXT format": {
212-
filePath: filepath.Join(testTextDataPath, invalidFormatTEXTTestFile),
203+
fileName: "invalid-format.log",
204+
testData: `2024-03-04T17:55:08Z INFO {metricName: accuracy, metricValue: 0.6752
205+
2024-03-04T17:55:08Z INFO {metricName: loss, metricValue: 0.3634}`,
213206
filters: []string{"{metricName: ([\\w|-]+), metricValue: ((-?\\d+)(\\.\\d+)?)}"},
214207
metrics: []string{"accuracy", "loss"},
215208
fileFormat: commonv1beta1.TextFormat,
@@ -226,7 +219,9 @@ func TestCollectObservationLog(t *testing.T) {
226219
},
227220
},
228221
"Invalid timestamp for logs in JSON format": {
229-
filePath: filepath.Join(testJsonDataPath, invalidTimestampJSONTestFile),
222+
fileName: "invalid-timestamp.json",
223+
testData: `{"checkpoint_path": "", "global_step": "0", "loss": "0.22082142531871796", "timestamp": "invalid", "trial": "0"}
224+
{"acc": "0.9349666833877563", "checkpoint_path": "", "global_step": "0", "timestamp": 1638422847, "trial": "0"}`,
230225
fileFormat: commonv1beta1.JsonFormat,
231226
metrics: []string{"acc", "loss"},
232227
expected: &v1beta1.ObservationLog{
@@ -249,7 +244,9 @@ func TestCollectObservationLog(t *testing.T) {
249244
},
250245
},
251246
"Invalid timestamp for logs in TEXT format": {
252-
filePath: filepath.Join(testTextDataPath, invalidTimestampTEXTTestFile),
247+
fileName: "invalid-timestamp.log",
248+
testData: `2024-03-04T17:55:08Z INFO {metricName: accuracy, metricValue: 0.6752}
249+
invalid INFO {metricName: loss, metricValue: 0.3634}`,
253250
metrics: []string{"accuracy", "loss"},
254251
filters: []string{"{metricName: ([\\w|-]+), metricValue: ((-?\\d+)(\\.\\d+)?)}"},
255252
fileFormat: commonv1beta1.TextFormat,
@@ -273,7 +270,10 @@ func TestCollectObservationLog(t *testing.T) {
273270
},
274271
},
275272
"Missing objective metric in JSON training logs": {
276-
filePath: filepath.Join(testJsonDataPath, missingMetricJSONTestFile),
273+
fileName: "missing-objective-metric.json",
274+
testData: `{"checkpoint_path": "", "global_step": "0", "loss": "0.22082142531871796", "timestamp": 1638422847.28721, "trial": "0"}
275+
{"checkpoint_path": "", "global_step": "1", "loss": "0.1414974331855774", "timestamp": "2021-12-02T14:27:50.000035161+09:00", "trial": "0"}
276+
{"checkpoint_path": "", "global_step": "2", "loss": "0.10683439671993256", "trial": "0"}`,
277277
fileFormat: commonv1beta1.JsonFormat,
278278
metrics: []string{"acc", "loss"},
279279
expected: &v1beta1.ObservationLog{
@@ -289,7 +289,9 @@ func TestCollectObservationLog(t *testing.T) {
289289
},
290290
},
291291
"Missing objective metric in TEXT training logs": {
292-
filePath: filepath.Join(testTextDataPath, missingMetricTEXTTestFile),
292+
fileName: "missing-objective-metric.log",
293+
testData: `2024-03-04T17:55:08Z INFO {metricName: loss, metricValue: 0.3634}
294+
2024-03-04T17:55:08Z INFO {metricName: loss, metricValue: 0.8671}`,
293295
fileFormat: commonv1beta1.TextFormat,
294296
metrics: []string{"accuracy", "loss"},
295297
expected: &v1beta1.ObservationLog{
@@ -306,126 +308,21 @@ func TestCollectObservationLog(t *testing.T) {
306308
},
307309
}
308310

311+
tmpDir := t.TempDir()
309312
for name, test := range testCases {
310313
t.Run(name, func(t *testing.T) {
311-
actual, err := CollectObservationLog(test.filePath, test.metrics, test.filters, test.fileFormat)
312-
if (err != nil) != test.err {
313-
t.Errorf("\nGOT: \n%v\nWANT: %v\n", err, test.err)
314-
} else {
315-
if diff := cmp.Diff(test.expected, actual); diff != "" {
316-
t.Errorf("Unexpected parsed result (-want,+got):\n%s", diff)
314+
if test.testData != "" {
315+
if err := os.WriteFile(filepath.Join(tmpDir, test.fileName), []byte(test.testData), 0600); err != nil {
316+
t.Fatalf("failed to write test data: %v", err)
317317
}
318318
}
319+
actual, err := CollectObservationLog(filepath.Join(tmpDir, test.fileName), test.metrics, test.filters, test.fileFormat)
320+
if diff := cmp.Diff(test.wantError, err, cmpopts.EquateErrors()); len(diff) != 0 {
321+
t.Errorf("Unexpected error (-want,+got):\n%s", diff)
322+
}
323+
if diff := cmp.Diff(test.expected, actual); len(diff) != 0 {
324+
t.Errorf("Unexpected parsed result (-want,+got):\n%s", diff)
325+
}
319326
})
320327
}
321328
}
322-
323-
func generateTestDirs() error {
324-
// Generate JSON files' dir
325-
if _, err := os.Stat(testJsonDataPath); err != nil {
326-
if err = os.MkdirAll(testJsonDataPath, 0700); err != nil {
327-
return err
328-
}
329-
}
330-
331-
// Generate TEXT files' dir
332-
if _, err := os.Stat(testTextDataPath); err != nil {
333-
if err = os.MkdirAll(testTextDataPath, 0700); err != nil {
334-
return err
335-
}
336-
}
337-
338-
return nil
339-
}
340-
341-
func generateJSONTestFiles() error {
342-
testData := []struct {
343-
fileName string
344-
data string
345-
}{
346-
{
347-
fileName: validJSONTestFile,
348-
data: `{"checkpoint_path": "", "global_step": "0", "loss": "0.22082142531871796", "timestamp": 1638422847.28721, "trial": "0"}
349-
{"acc": "0.9349666833877563", "checkpoint_path": "", "global_step": "0", "timestamp": 1638422847.287801, "trial": "0"}
350-
{"checkpoint_path": "", "global_step": "1", "loss": "0.1414974331855774", "timestamp": "2021-12-02T14:27:50.000035161Z", "trial": "0"}
351-
{"acc": "0.9586416482925415", "checkpoint_path": "", "global_step": "1", "timestamp": "2021-12-02T14:27:50.000037459Z", "trial": "0"}
352-
{"checkpoint_path": "", "global_step": "2", "loss": "0.10683439671993256", "trial": "0"}
353-
`,
354-
},
355-
{
356-
fileName: invalidFormatJSONTestFile,
357-
data: `"checkpoint_path": "", "global_step": "0", "loss": "0.22082142531871796", "timestamp": 1638422847.28721, "trial": "0"
358-
{"acc": "0.9349666833877563", "checkpoint_path": "", "global_step": "0", "timestamp": 1638422847.287801, "trial": "0
359-
`,
360-
},
361-
{
362-
fileName: invalidTimestampJSONTestFile,
363-
data: `{"checkpoint_path": "", "global_step": "0", "loss": "0.22082142531871796", "timestamp": "invalid", "trial": "0"}
364-
{"acc": "0.9349666833877563", "checkpoint_path": "", "global_step": "0", "timestamp": 1638422847, "trial": "0"}
365-
`,
366-
}, {
367-
fileName: missingMetricJSONTestFile,
368-
data: `{"checkpoint_path": "", "global_step": "0", "loss": "0.22082142531871796", "timestamp": 1638422847.28721, "trial": "0"}
369-
{"checkpoint_path": "", "global_step": "1", "loss": "0.1414974331855774", "timestamp": "2021-12-02T14:27:50.000035161+09:00", "trial": "0"}
370-
{"checkpoint_path": "", "global_step": "2", "loss": "0.10683439671993256", "trial": "0"}`,
371-
},
372-
}
373-
374-
for _, td := range testData {
375-
filePath := filepath.Join(testJsonDataPath, td.fileName)
376-
if err := os.WriteFile(filePath, []byte(td.data), 0600); err != nil {
377-
return err
378-
}
379-
}
380-
381-
return nil
382-
}
383-
384-
func generateTEXTTestFiles() error {
385-
testData := []struct {
386-
fileName string
387-
data string
388-
}{
389-
{
390-
fileName: validTEXTTestFile,
391-
data: `2024-03-04T17:55:08Z INFO {metricName: accuracy, metricValue: 0.8078};{metricName: loss, metricValue: 0.5183}
392-
2024-03-04T17:55:08Z INFO {metricName: accuracy, metricValue: 0.6752}
393-
2024-03-04T17:55:08Z INFO {metricName: loss, metricValue: 0.3634}
394-
2024-03-04T17:55:08Z INFO {metricName: accuracy, metricValue: 100}
395-
2024-03-04T17:55:08Z INFO {metricName: accuracy, metricValue: 888.333}
396-
2024-03-04T17:55:08Z INFO {metricName: accuracy, metricValue: -0.4759}
397-
{metricName: loss, metricValue: 0.8671}`,
398-
},
399-
{
400-
fileName: invalidValueTEXTTestFile,
401-
data: `2024-03-04T17:55:08Z INFO {metricName: accuracy, metricValue: .333}
402-
2024-03-04T17:55:08Z INFO {metricName: accuracy, metricValue: -.333}
403-
2024-03-04T17:55:08Z INFO {metricName: accuracy, metricValue: - 345.333}
404-
2024-03-04T17:55:08Z INFO {metricName: accuracy, metricValue: 888.}`,
405-
},
406-
{
407-
fileName: invalidFormatTEXTTestFile,
408-
data: `2024-03-04T17:55:08Z INFO {metricName: accuracy, metricValue: 0.6752
409-
2024-03-04T17:55:08Z INFO {metricName: loss, metricValue: 0.3634}`,
410-
},
411-
{
412-
fileName: invalidTimestampTEXTTestFile,
413-
data: `2024-03-04T17:55:08Z INFO {metricName: accuracy, metricValue: 0.6752}
414-
invalid INFO {metricName: loss, metricValue: 0.3634}`,
415-
},
416-
{
417-
fileName: missingMetricTEXTTestFile,
418-
data: `2024-03-04T17:55:08Z INFO {metricName: loss, metricValue: 0.3634}
419-
2024-03-04T17:55:08Z INFO {metricName: loss, metricValue: 0.8671}`,
420-
},
421-
}
422-
423-
for _, td := range testData {
424-
filePath := filepath.Join(testTextDataPath, td.fileName)
425-
if err := os.WriteFile(filePath, []byte(td.data), 0600); err != nil {
426-
return err
427-
}
428-
}
429-
430-
return nil
431-
}

0 commit comments

Comments
 (0)