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

inject clock for usage in isDateInTolerance #3037

Merged
merged 1 commit into from
Jul 30, 2019

Conversation

christophsturm
Copy link
Contributor

No description provided.

@christophsturm christophsturm requested a review from ripcurlx as a code owner July 30, 2019 13:29
@mrosseel
Copy link
Contributor

Clock doesn't seem to be used yet, are those commits coming?

@christophsturm
Copy link
Contributor Author

i have a different PR that adds unit tests for the SignedWitness class. once this is merged i will use the clock for more unit testing. I just wanted to do the clock injection in a separate pr to make prs small and easy to review and not block a bigger pr if someone has an objection.

@sqrrm
Copy link
Member

sqrrm commented Jul 30, 2019

For these kinds of PRs it would be good to have an explanation of why they're needed. It's not obvious why this one is needed without the follow up conversation.

Copy link
Member

@sqrrm sqrrm left a comment

Choose a reason for hiding this comment

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

utACK

@sqrrm sqrrm merged commit 360dc0b into bisq-network:master Jul 30, 2019
@christophsturm
Copy link
Contributor Author

For these kinds of PRs it would be good to have an explanation of why they're needed. It's not obvious why this one is needed without the follow up conversation.

ok.

julianknutsen added a commit to julianknutsen/bisq that referenced this pull request Nov 7, 2019
Convert the isDateInTolerance code to use the already available clock
instead of using new Date().getTime(). This allow finer control of time
in tests and finishes the intention of bisq-network#3037.
julianknutsen added a commit to julianknutsen/bisq that referenced this pull request Nov 7, 2019
Attach a Clock object that can be used instead of System.currentTimeMillis()
so that we have more control over the testing of time-sensitive code.
Specifically, the code around expiration.

This involves attaching a Clock to the resolver
so all fromProto methods have one available when they
reconstruct a message. This uses the Injector for the APP
and a default Clock.systemDefaultZone is used in the manual
instantiations.

Work was already done in bisq-network#3037 to make this possible.

All tests still use the default system clock for now.
julianknutsen added a commit to julianknutsen/bisq that referenced this pull request Nov 7, 2019
Attach a Clock object that can be used instead of System.currentTimeMillis()
so that we have more control over the testing of time-sensitive code.
Specifically, the code around expiration.

This involves attaching a Clock to the resolver
so all fromProto methods have one available when they
reconstruct a message. This uses the Injector for the APP
and a default Clock.systemDefaultZone is used in the manual
instantiations.

Work was already done in bisq-network#3037 to make this possible.

All tests still use the default system clock for now.
julianknutsen added a commit to julianknutsen/bisq that referenced this pull request Nov 13, 2019
Switch from System.currentTimeMills() to
Clock.millis() so dependency injection can
be used for tests that need finer control of time.

This involves attaching a Clock to the resolver
so all fromProto methods have one available when they
reconstruct a message. This uses the Injector for the APP
and a default Clock.systemDefaultZone is used in the manual
instantiations.

Work was already done in bisq-network#3037 to make this possible.

All tests still use the default system clock for now.
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.

3 participants