CORE-18879: Ensure verifying notary implementation #5425
Conversation
|
Jenkins build for PR 5425 build 8 Build Successful: |
jennyang-r3
force-pushed
the
jennyang/CORE-17807-ensure-verifying-notary-impl
branch
from
30e4468 to
25eb26d
Compare
There was a problem hiding this comment.
I have few questions/remarks about the tests in this PR, see the comments inline, also:
- Why is the ordering of the tests required? Does it make a difference in which order the tests are run? If yes, that is a bad code smell, if no, can we get rid of it?
- A lot of these tests just check that exceptions thrown by other services are converted correctly - they do not check that the real thing would behave as expected on wrong or malicious inputs, so I think we still need to test most of these things as e2e tests. This is especially true for any test where you set a mock API to throw.
...com/r3/corda/notary/plugin/contractverifying/server/ContractVerifyingNotaryServerFlowImpl.kt
Outdated
Show resolved
Hide resolved
| @@ -299,8 +401,13 @@ class ContractVerifyingNotaryServerFlowImplTest { | |||
| } | |||
|
|
|||
| @Test | |||
| @Order(10) | |||
| fun `Contract verifying notary plugin server should respond with error if transaction verification fails`() { | |||
There was a problem hiding this comment.
That title is wrong - it suggests that the transaction verification (i.e. of the initital transaction) fails - but it really is the Merkle proof verification of the dependencies that fails here.
|
|
||
| @Test | ||
| @Order(16) | ||
| fun `Contract verifying notary should respond with error if either current or filtered tx is invalid`() { |
There was a problem hiding this comment.
I don't understand the test case - in the real world, the initial transaction cannot be a dependency of itself, as it's not a valid, notarized transaction at this point yet.
If anything, this should complain that there is a circular dependency.
...r3/corda/notary/plugin/contractverifying/server/ContractVerifyingNotaryServerFlowImplTest.kt
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Test | ||
| @Order(Integer.MAX_VALUE) |
There was a problem hiding this comment.
Why does this test need to be run last?
There was a problem hiding this comment.
Ordered test sorted out
...r3/corda/notary/plugin/contractverifying/server/ContractVerifyingNotaryServerFlowImplTest.kt
Outdated
Show resolved
Hide resolved
jennyang-r3
force-pushed
the
jennyang/CORE-17807-ensure-verifying-notary-impl
branch
from
53bdbd1 to
1910567
Compare
jennyang-r3
force-pushed
the
jennyang/CORE-17807-ensure-verifying-notary-impl
branch
from
1910567 to
429cdd4
Compare
|
jennyang-r3
changed the title
jennyang-r3
deleted the
jennyang/CORE-17807-ensure-verifying-notary-impl
branch
In this PR, I added more unit test cases for verifying notary, and checking any missing checks, and refactored the test.
added tests: