JSON schema to validate actor ls #326

acruikshank · 2018-04-23T16:38:35Z

Addresses https://app.zenhub.com/workspace/o/filecoin-project/go-filecoin/issues/241.

This PR adds a JSON schema file to the project and a test that validates entries returned by the actor/ls command coming back from the server are valid.

Notes

This PR adds gojsonschema as a testing dependency. We also update go-ipfs-cmds to the head of the feat/filecoin branch to address an unmarshalling bug.

phritz

@whyrusleeping is your primary stakeholder here i believe.

phritz · 2018-04-23T22:10:50Z

build/main.go

@@ -50,6 +50,7 @@ func deps() {
 		"go get -u github.com/alecthomas/gometalinter",
 		"gometalinter --install",
 		"go get -u github.com/stretchr/testify",
+		"go get -u github.com/xeipuuv/gojsonschema",


is there a reason we don't want to gx this dependency?

@phritz What is the process by which we gx a GitHub-hosted project that PL didn't author?

This import follows the pattern we've used for the github.com/stretchr/testify. I went part way down the path of gxing this dependency, but it felt kind of wrong for code used only for testing. The package doesn't have a .gx version in github so it will be challenging to maintain it. I'd be happy to add it to package.json, but I'd like some guidance that it's actually appropriate.

no that's the explanation i was looking for

phritz · 2018-04-23T22:42:01Z

commands/actor.go

@@ -32,6 +32,12 @@ var actorLsCmd = &cmds.Command{
 	Type: &actorView{},
 	Encoders: cmds.EncoderMap{
 		cmds.JSON: cmds.MakeTypedEncoder(func(req *cmds.Request, w io.Writer, a *actorView) error {
+			// go-ipfs-cmds decodes each json obj from the daemon into the same struct.
+			// This sometimes causes actors to show fields from actors we've already emitted.
+			// This is a work-around pending a fix in the go-ipfs-cmds codebase.


most useful if you can include a link that way people don't have to go searching for the issue/fix

phritz · 2018-04-23T22:56:03Z

commands/actor_test.go

+
+		assert.True(result.Valid())
+		for _, desc := range result.Errors() {
+			fmt.Printf("- %s\n", desc)


is it maybe better to use assert.Fail or similar instead of printfing here and elsewhere?

Good point.

laser

Overall, lookin' good. You put a lot of work into those schemas - and nice bug catch.

My request: I think you should run the actor ls command and assert that its output conforms to some schema instead of what you're doing now, which is to skip the command-running stuff and encode a single actorView to JSON. If using a JSON schema file to validate NDJSON isn't possible, then I suggest capturing the output of a command, splitting its output on newlines and running those newline-delimited JSON objects through your JSON schema-validator.

laser · 2018-04-24T04:13:06Z

commands/actor_test.go

+	"github.com/stretchr/testify/require"
+	"github.com/xeipuuv/gojsonschema"
+	"gx/ipfs/QmcZfnkapfECQGcLZaf9B79NRg7cRa9EnZh4LSbkCzwNvY/go-cid"
+	"gx/ipfs/QmdtiofXbibTe6Day9ii5zjBZpSRm8vhfoerrNuY3sAQ7e/go-hamt-ipld"


It's likely that nobody told you this, but we have a convention for organizing dependencies in Golang source code.

laser · 2018-04-24T04:26:12Z

commands/actor_test.go

+		return gojsonschema.Validate(schemaLoader, jsonLoader)
+	}
+
+	t.Run("Emitted AccountActor JSON conforms to schema", func(t *testing.T) {


I see that you're testing that the JSON that we create from an actorView conforms to our JSON schema - but I don't see that you're testing that this is the JSON that we emit from any command.

This distinction is important: The intent of the original story was to ensure that the JSON that we returned from a command conformed to some schema. We created that story because of a bug I introduced to the dag get command's implementation which broke the Filecoin Explorer but did not change the way that we were encoding Go values to JSON.

laser · 2018-04-24T04:28:07Z

commands/schema/actor_ls.schema.json

@@ -0,0 +1,157 @@
+{
+  "definitions": {


Nice work. Creating these things by hand is a pain.

Question: Are you aware of any way to use a JSON schema file to validate some NDJSON?

seems like we could just autogenerate these.

also relevant: ipfs/go-ipfs-cmds#94

We can autogenerate this schema, but there are some reasons why I chose not to:

Writing JSON schema isn't very hard.

JSON Schema has a lot of flexibility how it's structured. It's easy to write (or generate) a version that accurately validates, but provides no meaningful feedback on which element failed. I went through a few variations to get a design that worked well.

An autogenerated schema can, at best, validate that a particular document conforms to one particular permutation of all possible ways to assemble the types. Specifying where content is required and how types relate are things that requires some understanding of the code's meaning.

acruikshank · 2018-04-24T15:16:24Z

@laser Re: assertions on actor ls. I've added schema validation in TestActorDaemon which is as close as we can really get to testing the actual command.

laser · 2018-04-24T16:08:04Z

@acruikshank

@laser Re: assertions on actor ls. I've added schema validation in TestActorDaemon which is as close as we can really get to testing the actual command.

Looking good! I just approved the PR.

acruikshank · 2018-04-25T03:30:20Z

@phritz and @whyrusleeping I think I've addressed most of your concerns. Could you take another look?

phritz

I think having something like these schema tests is really important. With why's approval let's adopt this as policy now unless he has an easier idea. If we do then update https://github.com/filecoin-project/go-filecoin/blob/master/CONTRIBUTING.md#testing-philosophy to point to this example as canonical.

BTW you probably have too many reviewers.

whyrusleeping · 2018-04-26T10:03:45Z

seems fine to me. Didn't review thoroughly, but nothing blocking over here.

update go-ipfs-cmds to version with unmarshalling fix and remove workaround

acruikshank force-pushed the feat/add_json_schema_test branch from 5e18292 to bbb2175 Compare April 23, 2018 17:14

acruikshank changed the title ~~JSON schema to validate actor ls~~ [WIP] JSON schema to validate actor ls Apr 23, 2018

acruikshank force-pushed the feat/add_json_schema_test branch from 51301fc to 426a4be Compare April 23, 2018 17:59

acruikshank requested review from phritz and dignifiedquire April 23, 2018 18:27

acruikshank changed the title ~~[WIP] JSON schema to validate actor ls~~ JSON schema to validate actor ls Apr 23, 2018

phritz reviewed Apr 23, 2018

View reviewed changes

laser suggested changes Apr 24, 2018

View reviewed changes

acruikshank force-pushed the feat/add_json_schema_test branch from aa62fc3 to c054b1b Compare April 24, 2018 13:22

laser approved these changes Apr 24, 2018

View reviewed changes

acruikshank force-pushed the feat/add_json_schema_test branch from a852ee0 to 7246fd5 Compare April 24, 2018 18:09

phritz approved these changes Apr 25, 2018

View reviewed changes

acruikshank force-pushed the feat/add_json_schema_test branch from 7246fd5 to b8959fd Compare April 25, 2018 23:44

schema validates actor ls

fd76d92

update go-ipfs-cmds to version with unmarshalling fix and remove workaround

acruikshank force-pushed the feat/add_json_schema_test branch from b8959fd to fd76d92 Compare April 26, 2018 13:13

acruikshank merged commit 80a31fc into master Apr 26, 2018

dignifiedquire deleted the feat/add_json_schema_test branch April 27, 2018 10:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JSON schema to validate actor ls #326

JSON schema to validate actor ls #326

acruikshank commented Apr 23, 2018 •

edited

Loading

phritz left a comment

phritz Apr 23, 2018

laser Apr 24, 2018

acruikshank Apr 24, 2018

phritz Apr 25, 2018

phritz Apr 23, 2018

phritz Apr 23, 2018

acruikshank Apr 24, 2018

laser left a comment

laser Apr 24, 2018

laser Apr 24, 2018

laser Apr 24, 2018

whyrusleeping Apr 24, 2018

whyrusleeping Apr 24, 2018

acruikshank Apr 24, 2018

acruikshank commented Apr 24, 2018

laser commented Apr 24, 2018

acruikshank commented Apr 25, 2018

phritz left a comment

whyrusleeping commented Apr 26, 2018

JSON schema to validate actor ls #326

JSON schema to validate actor ls #326

Conversation

acruikshank commented Apr 23, 2018 • edited Loading

Notes

phritz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acruikshank commented Apr 24, 2018

laser commented Apr 24, 2018

acruikshank commented Apr 25, 2018

phritz left a comment

Choose a reason for hiding this comment

whyrusleeping commented Apr 26, 2018

acruikshank commented Apr 23, 2018 •

edited

Loading