Initial testing PR #25

jn9e9 · 2021-02-24T23:20:39Z

Add low level interface tests. Add initial operation client data driven tests and test generator.

Change content type to zero to match rust code.

Partial completion of #12

Signed-off-by: John 9e9 [email protected]

…en tests and test generator. Change content type to zero to match rust code. Signed-off-by: John 9e9 <[email protected]>

Signed-off-by: John 9e9 <[email protected]>

hug-dev

Thanks a lot for adding all this 🤩
Let's start a discussion for the review. On a high-level view, this PR adds:

a Rust tool crate to generate JSON test cases from NativeOperation, NativeResult and their serialisations
Go test files checking that for each operation, the correct serialisation/deserialisation happens correctly on both dorection
Some adjustments to the wire protocol
Tests on the wire protocol header (de)serialisation
Tests on the authenticators and the (de)serialisation of the ID

Is that correct? Feel free to add more independant categories if I missed things.

1 and 2 seem to be the big pieces which might need more discussion. The rest seems to be easier to review and get merged. Where I want to go is that if we come with things in this PR that are independant with 1 and 2, perhaps you could send a separate PR for those, which we first agree and merge. Then we can focus the discussion on the meaty stuff!

hug-dev · 2021-03-01T10:00:36Z

.github/workflows/build.yaml

+      # - name: Upload Coverage report to CodeCov
+      #   uses: codecov/[email protected]
+      #   with:
+      #     token: ${{secrets.CODECOV_TOKEN}}
+      #     file: ./coverage.txt


@ionut-arm has been enabling code coverage on multiple of our repos recently, so I guess we could do it here as well now 😃

yes, that'd be good if somebody could sign up with codecov.io account for the project, get a token then make it available.

I have it on my fork, but have disabled it for the PR as it causes the build to fail without the secret set.

Will create an issue for it.

have created #27

hug-dev · 2021-03-01T10:01:20Z

.gitignore

@@ -1,3 +1,4 @@
 node_modules
 .vscode
 ...
+test-notes.md


Is it a file generated during the test process?

nope, good spot, that was a file i was keeping track of the tests with.... will remove

hug-dev · 2021-03-01T10:49:58Z

interface/operations/operations_client.go

+		provider: requests.ProviderCore,
+		authType: auth.AuthUnixPeerCredentials,


In the future, I guess those two fields will be computed via #6 ?

yes they would

hug-dev · 2021-03-01T10:53:14Z

interface/operations/operations_client.go

@@ -522,6 +528,8 @@ func (c Client) operation(op requests.OpCode, provider requests.ProviderID, requ
 	if err != nil {
 		return err
 	}
+	// TODO ensure that we continue writing whole buffer afer a short write
+	// https://github.com/parallaxsecond/parsec-client-go/issues/23


As in the following call is asynchronous? It would be possible for it to return successfully when not all the b.Bytes() have been written?
Is it possible to make it fully blocking at first?

hug-dev · 2021-03-01T11:07:34Z

interface/operations/test/operations_client_test.go

+type TestCase struct {
+	Name                  string      `json:"name"`
+	RequestData           interface{} `json:"request_data"`
+	ExpectedRequestBinary string      `json:"expected_request_binary"`
+	ResponseBinary        string      `json:"response_binary"`
+	ExpectedResponseData  interface{} `json:"expected_response"`
+	ExpectSuccess         bool        `json:"expect_success"`
+}


Just for my understanding, with those tests you check that the stack correctly performs the conversions:

from high-level functions with specific parameters (in RequestData) to binary data transmitted

from the binary response to the high-level functions return value

yes, that's correct, but a comment would be a good idea ;)

hug-dev · 2021-03-01T11:10:34Z

interface/requests/mod.go

@@ -37,3 +35,7 @@ const (
 	OpPsaVerifyMessage     OpCode = 0x0019
 	OpListKeys             OpCode = 0x001A


Maybe we need a warning notice here, or a test of some sort, to also modify the IsValid method is an opcode bigger than the biggest one is added.
Same for all the IsValid methods 😬

jn9e9 · 2021-03-01T23:29:56Z

Thanks a lot for adding all this 🤩
Let's start a discussion for the review. On a high-level view, this PR adds:
1. a Rust tool crate to generate JSON test cases from `NativeOperation`, `NativeResult` and their serialisations

2. Go test files checking that for each operation, the correct serialisation/deserialisation happens correctly on both dorection

3. Some adjustments to the wire protocol

4. Tests on the wire protocol header (de)serialisation

5. Tests on the authenticators and the (de)serialisation of the ID
Is that correct? Feel free to add more independant categories if I missed things.
Yes, that is about it

1 and 2 seem to be the big pieces which might need more discussion. The rest seems to be easier to review and get merged. Where I want to go is that if we come with things in this PR that are independant with 1 and 2, perhaps you could send a separate PR for those, which we first agree and merge. Then we can focus the discussion on the meaty stuff!
OK, I'll see if i can create a new branch and pull the 3-5 changes across and resubmit another PR to cover those.

Signed-off-by: John 9e9 <[email protected]>

jn9e9 · 2021-03-01T23:35:37Z

Did wonder if the test data generator (and the test data) would be best in the parsec-interface-rs repo or its own. We could either do it now, or wait until I've done a lump more testing and see how it evolves first?

hug-dev · 2021-03-02T09:13:50Z

OK, I'll see if i can create a new branch and pull the 3-5 changes across and resubmit another PR to cover those.

That sounds good!

Did wonder if the test data generator (and the test data) would be best in the parsec-interface-rs repo or its own. We could either do it now, or wait until I've done a lump more testing and see how it evolves first?

That is an interesting question... Let's open that one on the community channel to see what everyone thinks.

hug-dev · 2021-03-02T12:07:01Z

Let's open that one on the community channel to see what everyone thinks.

See the linked issue for a proposal 😸 If you can set aside a few hours to read it

jn9e9 · 2021-03-10T20:49:06Z

going to close this as will be ote with acceptance/implementation of this: https://github.com/parallaxsecond/parsec/issues/350

Retaining branch on fork for reference

jn9e9 added 3 commits February 24, 2021 23:13

Add low level interface tests. Add initial operation client data driv…

d3f468c

…en tests and test generator. Change content type to zero to match rust code. Signed-off-by: John 9e9 <[email protected]>

fix failing test

2571766

Signed-off-by: John 9e9 <[email protected]>

disable code coverage

a7f51d7

Signed-off-by: John 9e9 <[email protected]>

ionut-arm self-requested a review February 26, 2021 00:44

hug-dev reviewed Mar 1, 2021

View reviewed changes

clean up .gitignre

d5b7123

Signed-off-by: John 9e9 <[email protected]>

hug-dev mentioned this pull request Mar 3, 2021

Merge wireprotocol and auth tests and wireprotocol fixes from PR#25 branch #28

Merged

jn9e9 closed this Mar 10, 2021

jn9e9 deleted the master branch March 10, 2021 22:06

hug-dev mentioned this pull request Apr 21, 2021

Proposal: testing clients with a mock service parallaxsecond/parsec-mock#2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial testing PR #25

Initial testing PR #25

jn9e9 commented Feb 24, 2021

hug-dev left a comment

hug-dev Mar 1, 2021

jn9e9 Mar 1, 2021 •

edited

Loading

jn9e9 Mar 1, 2021

hug-dev Mar 1, 2021

jn9e9 Mar 1, 2021

hug-dev Mar 1, 2021

jn9e9 Mar 1, 2021

hug-dev Mar 1, 2021

hug-dev Mar 1, 2021

jn9e9 Mar 1, 2021

hug-dev Mar 1, 2021

jn9e9 commented Mar 1, 2021

jn9e9 commented Mar 1, 2021

hug-dev commented Mar 2, 2021

hug-dev commented Mar 2, 2021

jn9e9 commented Mar 10, 2021

		provider: requests.ProviderCore,
		authType: auth.AuthUnixPeerCredentials,

		@@ -37,3 +35,7 @@ const (
		OpPsaVerifyMessage OpCode = 0x0019
		OpListKeys OpCode = 0x001A

Initial testing PR #25

Initial testing PR #25

Conversation

jn9e9 commented Feb 24, 2021

hug-dev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jn9e9 Mar 1, 2021 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jn9e9 commented Mar 1, 2021

jn9e9 commented Mar 1, 2021

hug-dev commented Mar 2, 2021

hug-dev commented Mar 2, 2021

jn9e9 commented Mar 10, 2021

jn9e9 Mar 1, 2021 •

edited

Loading