Thanks for the review, replies are inline below. On 2019.07.11 15:35, Jakub Narebski wrote: > Josh Steadmon <steadmon@xxxxxxxxxx> writes: > > > trace_schema_validator can be used to verify that trace2 event output > > conforms to the expectations set by the API documentation and codified > > in event_schema.json (or strict_schema.json). This allows us to build a > > regression test to verify that trace2 output does not change > > unexpectedly. > > > > Signed-off-by: Josh Steadmon <steadmon@xxxxxxxxxx> > > Very nitpicky comments below. > > > --- > > t/trace_schema_validator/.gitignore | 1 + > > t/trace_schema_validator/Makefile | 10 +++ > > .../trace_schema_validator.go | 78 +++++++++++++++++++ > > 3 files changed, 89 insertions(+) > > create mode 100644 t/trace_schema_validator/.gitignore > > create mode 100644 t/trace_schema_validator/Makefile > > create mode 100644 t/trace_schema_validator/trace_schema_validator.go > > > > diff --git a/t/trace_schema_validator/.gitignore b/t/trace_schema_validator/.gitignore > > new file mode 100644 > > index 0000000000..c3f1e04e9e > > --- /dev/null > > +++ b/t/trace_schema_validator/.gitignore > > @@ -0,0 +1 @@ > > +trace_schema_validator > > diff --git a/t/trace_schema_validator/Makefile b/t/trace_schema_validator/Makefile > > new file mode 100644 > > index 0000000000..ed22675e5d > > --- /dev/null > > +++ b/t/trace_schema_validator/Makefile > > @@ -0,0 +1,10 @@ > > +.PHONY: fetch_deps clean > > + > > +trace_schema_validator: fetch_deps trace_schema_validator.go > > + go build > > I don't know the Go build process, but shouldn't the name of target and > the name of actual source file passed to the command? > > Though I don't think we would _need_ for example being able to configure > Go build process via Makefile variables, like e.g. $(GOBUILD) in > https://sohlich.github.io/post/go_makefile/ Yeah it seems optional for Go, but fixed to make things more consistent with the other makefiles. > > + > > +fetch_deps: > > + go get github.com/xeipuuv/gojsonschema > > + > > +clean: > > + rm -f trace_schema_validator > > In git Makefile we use > > clean: > $(RM) $(PROGRAMS) > > I'm not sure if it is needed for operating system independence, but > using $(RM) is a standard way to create 'clean' targets... Fixed in V3. > > diff --git a/t/trace_schema_validator/trace_schema_validator.go > > b/t/trace_schema_validator/trace_schema_validator.go > > new file mode 100644 > > index 0000000000..f779ac5ff5 > > --- /dev/null > > +++ b/t/trace_schema_validator/trace_schema_validator.go > > @@ -0,0 +1,78 @@ > > +// trace_schema_validator validates individual lines of an input file against a > > +// provided JSON-Schema for git trace2 event output. > > +// > > +// Traces can be collected by setting the GIT_TRACE2_EVENT environment variable > > +// to an absolute path and running any Git command; traces will be appended to > > +// the file. > > +// > > +// Traces can then be verified like so: > > +// trace_schema_validator \ > > +// --trace2_event_file /path/to/trace/output \ > > +// --schema_file /path/to/schema > > +package main > > + > > +import ( > > + "bufio" > > + "flag" > > + "log" > > + "os" > > + "path/filepath" > > + > > + "github.com/xeipuuv/gojsonschema" > > +) > > + > > +// Required flags > > +var schemaFile = flag.String("schema_file", "", "JSON-Schema filename") > > +var trace2EventFile = flag.String("trace2_event_file", "", "trace2 event filename") > > The standard for long options is to use "kebab case", not "snake case" > for them, i.e. --schema-file not current --schema_file, etc. Fixed in V3. > > + > > +func main() { > > + flag.Parse() > > + if *schemaFile == "" || *trace2EventFile == "" { > > + log.Fatal("Both --schema_file and --trace2_event_file are required.") > > + } > > I guess that you prefer required options with explicit arguments instead > of positional arguments (that is requiring the command to be called with > two arguments, first being schema file, second being event file to > validate). Yeah, that's the style I'm used to at work, but I can change this if positional args are more acceptable for Git. > > + schemaURI, err := filepath.Abs(*schemaFile) > > + if err != nil { > > + log.Fatal("Can't get absolute path for schema file: ", err) > > + } > > + schemaURI = "file://" + schemaURI > > + > > + schemaLoader := gojsonschema.NewReferenceLoader(schemaURI) > > + schema, err := gojsonschema.NewSchema(schemaLoader) > > + if err != nil { > > + log.Fatal("Problem loading schema: ", err) > > + } > > + > > + tracesFile, err := os.Open(*trace2EventFile) > > + if err != nil { > > + log.Fatal("Problem opening trace file: ", err) > > + } > > + defer tracesFile.Close() > > + > > + scanner := bufio.NewScanner(tracesFile) > > + > > + count := 0 > > + for ; scanner.Scan(); count++ { > > I see that you assume JSON-Lines format, i.e. one JSON object per line > in the file. Yeah, I'll add a comment noting this. This won't work with the provided list schemas unless we reformat everything to be on a single line. > > > + if count%10000 == 0 { > > + // Travis-CI expects regular output or it will time out. > > + log.Print("Validated items: ", count) > > I wonder if it wouldn't be better to provide --progress flag, which > Travis-CI job would turn on. Done, thanks for the suggestion. > > + } > > + event := gojsonschema.NewStringLoader(scanner.Text()) > > + result, err := schema.Validate(event) > > + if err != nil { > > + log.Fatal(err) > > + } > > + if !result.Valid() { > > + log.Print("Trace event is invalid: ", scanner.Text()) > > It might be good idea to print the line (i.e. the value of `count` > variable). > > I guess that conforming to the <filename>:<line>: prefix (like e.g. gcc > uses for errors) would be unnecessary. Done in V3. > > + for _, desc := range result.Errors() { > > + log.Print("- ", desc) > > + } > > + os.Exit(1) > > + } > > + } > > + > > + if err := scanner.Err(); err != nil { > > + log.Fatal("Scanning error: ", err) > > + } > > + > > + log.Print("Validated events: ", count) > > +}
