Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DRAFT] CORE-16353: Added flow tests using the Flow Testing Driver and fixed … #28

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

khutsijabari
Copy link

@khutsijabari khutsijabari commented Aug 31, 2023

…a bug on the Accept smart contract

Added flow tests using the Flow Testing Driver and fixed a bug on the Accept smart contract which I identified while executing the newly added flow tests. The bug was allowing proposers to accept their own proposals, for example, Alice issues £20 proposal to Bob, Bob modifies it to £30 and then accepts the modified proposal. Also. modified some of the dependencies to Corda 5.1 as that it what is compatible with the Flow Testing Driver. Obviously this code will not be merged until Corda 5.1 (and the Flow Testing Driver) is available publicly i.e. full GA. Also, updated the README.md file. Also, made changes to the flow classes to return the states' IDs instead of transaction IDs which were not being used anywhere. The returned states' IDs are required in flow tests to be able to test subsequent flows. Also, worth mentioning that I had to change the getter property for IsBuyer as for some weird reason Jackson renames it to just Buyer when serialising it and therefore breaking the flow tests.

…a bug on the Accept smart contract

Added flow tests using the Flow Testing Driver and fixed a bug on the Accept smart contract which I identified while executing the newly added flow tests. The bug was allowing proposers to accept their own proposals, for example, Alice issues £20 proposal to Bob, Bob modifies it to £20 and then accepts the modified proposal. Also. modified some of the dependencies to Corda 5.1 as that it what is compatible with the Flow Testing Driver. Obviously this code will not be merged until Corda 5.1 (and the Flow Testing Driver) is available publicly i.e. full GA. Also, updated the README.md file. Also, made changes to the flow classes to return the states' IDs instead of transaction IDs which were not being used anywhere. The returned states' IDs are required in flow tests to be able to test subsequent flows. Also, worth mentioning that I had to change the getter property for IsBuyer as for some weird reason Jackson renames it to just Buyer when serialising it and therefore breaking the flow tests.
@khutsijabari khutsijabari changed the title CORE-16353: Added flow tests using the Flow Testing Driver and fixed … [DRAFT] CORE-16353: Added flow tests using the Flow Testing Driver and fixed … Aug 31, 2023
Copy link

@tlawson3 tlawson3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Topic for discussion: Does this CorDapp sample have contract tests?
I'm wondering if the negative workflow tests you have would be better validated as contract tests - faster, focus attention to just contracts module tests and verify code?

@khutsijabari
Copy link
Author

Topic for discussion: Does this CorDapp sample have contract tests? I'm wondering if the negative workflow tests you have would be better validated as contract tests - faster, focus attention to just contracts module tests and verify code?

We have a separate JIRA for adding contract tests, same as Flow Testing Driver, those won't be merged until Contract Testing is available publicly. Flow Testing Driver also supports contract testing, this is to demonstrate that.

Addressed code review comments
… re-added them to workflows build.gradle file

Removed system properties from root build.gradle file and re-added them to workflows build.gradle file
tlawson3
tlawson3 previously approved these changes Aug 31, 2023
Removed unused Trade constructor and renamed Trade.ProposalId to Trade.TradeId because that is what it is. Updated the Accept flow request body in the ReadMe file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants