-
Notifications
You must be signed in to change notification settings - Fork 16
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
[BCI-3989][common] - CR methods err when service unstarted #705
Draft
Farber98
wants to merge
38
commits into
main
Choose a base branch
from
BCI-3989-cr-methods-error-when-unstarted
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from 30 commits
Commits
Show all changes
38 commits
Select commit
Hold shift + click to select a range
352adfe
check cr service is started
Farber98 173eca4
small refactor
Farber98 e16b694
Merge branch 'main' into BCI-3989-cr-methods-error-when-unstarted
Farber98 0d8a1b0
remove unused error
Farber98 af1fabf
bind before start
Farber98 4dcb052
add comments to tests
Farber98 4d966e6
startChainReader method
Farber98 5006bec
check methods return error when called without starting service
Farber98 cc3066b
Revert "check methods return error when called without starting service"
Farber98 b697ce2
service started checked working for both get latest value methods
Farber98 855b38c
close chain reader after tests
Farber98 fe91e30
Merge branch 'main' into BCI-3989-cr-methods-error-when-unstarted
Farber98 0cf190e
remove duplicate fields
Farber98 8e1388b
rename interface tester new methods
Farber98 72d3790
Merge branch 'main' into BCI-3989-cr-methods-error-when-unstarted
Farber98 215fb70
make start and close safe for concurrent use
Farber98 585ccc9
use atomic bool for isStarted flag
Farber98 81aaf5c
Merge branch 'main' into BCI-3989-cr-methods-error-when-unstarted
Farber98 1830e25
remove start as only closed is needed
Farber98 dd6363c
Merge branch 'main' into BCI-3989-cr-methods-error-when-unstarted
Farber98 dd26cd7
Merge branch 'main' into BCI-3989-cr-methods-error-when-unstarted
Farber98 50c55ef
Merge branch 'main' into BCI-3989-cr-methods-error-when-unstarted
Farber98 699bdf8
Merge branch 'main' into BCI-3989-cr-methods-error-when-unstarted
Farber98 fa1ace8
Merge branch 'main' into BCI-3989-cr-methods-error-when-unstarted
Farber98 bed6fbf
Merge branch 'main' into BCI-3989-cr-methods-error-when-unstarted
Farber98 208ef02
add start method to the interface. add start and close calls where ne…
Farber98 a453d2d
Merge branch 'main' into BCI-3989-cr-methods-error-when-unstarted
Farber98 0d7af20
fix conflicts with chain components pr
Farber98 647bdde
Merge branch 'main' into BCI-3989-cr-methods-error-when-unstarted
Farber98 489fd0a
fix pr conflicts
Farber98 e8ed82b
add flag to control if we return cr started
Farber98 66ba7cd
fix encodings iface
Farber98 392195f
Merge branch 'main' into BCI-3989-cr-methods-error-when-unstarted
Farber98 4e67d8d
refactor codec and chaincomponents ifaces
Farber98 54d7016
Merge branch 'main' into BCI-3989-cr-methods-error-when-unstarted
Farber98 e728084
Revert "Merge branch 'main' into BCI-3989-cr-methods-error-when-unsta…
Farber98 953d786
Reapply "Merge branch 'main' into BCI-3989-cr-methods-error-when-unst…
Farber98 f94c2c9
Merge branch 'main' into BCI-3989-cr-methods-error-when-unstarted
Farber98 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having trouble seeing why we need these helper methods.
impl
is already aContractReader
, so we don't need to cast it to a specific fake type:Additionally, there is already a
GetContractReader
method, so callers that are doing this:Could instead do this:
Or better yet:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmank88 @silaslenihan I couldn't do it this way. Tests fail for the loop relay client.
Pushed some changes with a new idea:
testCases
andSetup
to receive astartCR
flag and control whether we should start CR or notStart
andClose
will be handled bySetup
withCleanup
All tests are passing, including new ones where we test that the service does not start. Let me know your thoughts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please update the description? I'm having trouble understanding the fundamental problem we are trying to solve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the
BasicTester
interface is used more generally than for just contract reader, I don't think it makes sense to add a contract-reader-only argument toBasicTester.Setup
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fundamental problem we are trying to solve: We only want
GetLatestValue
,BatchGetLatestValue
andQueryKey
to be called when the serviceCR
is inStarted
state. IfCR
has not yet been started, we should return an error.To test this new behavior, we need a way to get the
CR
in both states (started
andnot started
).We can't just call
Start
andClose
directly on the return ofGetContractReader
to achieve this. This is because we also run relay client loop tests here. We are callingStart
andClose
on the client and not on the server as would be required.The two solutions I proposed were aimed at solving this problem, the one with the helper methods and now the one with the flag. I feel like the one with the flag is cleaner. We are providing in the
testCase
thestartCR
flag to determine if the underlyingSetup
function should return the service instarted
orunstarted
stateWhat do you think about defining a new interface similar to
BasicTester
but use it specifically oncontract-reader
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something isn't adding up here. The modification that I proposed to your helper methods was intended to execute exactly equivalently. It was not proposing a change in behavior - definitely not one that switched from client to server or vice versa. If the helper method solution was valid, then we should revisit it, but in the most simplified form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a new solution with interfaces refactor and comments on changes. If do you think it's okay we can go with it. Else, I'll revisit the one with the helper methods in the most simplified form