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

Feature/tests timeouts #287

Merged
merged 3 commits into from
Nov 30, 2018
Merged

Feature/tests timeouts #287

merged 3 commits into from
Nov 30, 2018

Conversation

Mingela
Copy link
Contributor

@Mingela Mingela commented Nov 27, 2018

Description of the Change

Added awkward integration tests timeouts.

Details:
junit-team/junit5#80 (comment)
https://blog.philipphauer.de/best-practices-unit-testing-kotlin/

On the one hand it is generally bad idea to use JUnit4 for Kotlin
On the other hand there is no convenient way to implement timeouts in JUnit5 yet

Usage Examples or Tests [optional]

@Mingela Mingela force-pushed the feature/tests-timeouts branch from 469efa4 to cf191d6 Compare November 27, 2018 10:02
@Mingela Mingela force-pushed the feature/tests-timeouts branch 2 times, most recently from bce0efb to 49d4f2b Compare November 27, 2018 10:09
@Mingela Mingela removed the request for review from Alexey-N-Chernyshov November 27, 2018 11:30
@Mingela Mingela changed the title Feature/tests timeouts Feature/tests timeouts (in progress) Nov 27, 2018
@Mingela Mingela force-pushed the feature/tests-timeouts branch 3 times, most recently from d3af0ef to b35dbb2 Compare November 27, 2018 13:47
@Mingela Mingela changed the title Feature/tests timeouts (in progress) Feature/tests timeouts Nov 27, 2018
@Mingela
Copy link
Contributor Author

Mingela commented Nov 27, 2018

Let me know if there is no sense in a such wrapping for some tests

@@ -25,3 +25,5 @@ test.ethereum.gasLimit=6000000
# --------- Bitcoin ---------
test.bitcoin.walletPath=deploy/bitcoin/test-btc.wallet
test.bitcoin.blockStoragePath=deploy/bitcoin/blocks
# --------- Timeout (minutes) ---------
test.testTimeout=5
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can hardcode this value.

Choose a reason for hiding this comment

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

Agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done as a constant value In ConfigHelper

newBalance
)
assertTrue(btcNotaryInitialization.isWatchedAddress(btcAddress))
assertTimeoutPreemptively(timeoutDuration) {
Copy link
Contributor

@dolgopolovwork dolgopolovwork Nov 28, 2018

Choose a reason for hiding this comment

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

I don't think we need assertTimeoutPreemptively in every test. Can we use this construction in tests known to be long-running only?

Copy link
Member

@Alexey-N-Chernyshov Alexey-N-Chernyshov left a comment

Choose a reason for hiding this comment

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

Please, use hardcoded testTimeout.

@Mingela Mingela force-pushed the feature/tests-timeouts branch 5 times, most recently from 2f9258f to 64cf7e6 Compare November 29, 2018 12:34
@Alexey-N-Chernyshov
Copy link
Member

Please, don't use force push for your branches, since it is impossible to view changes from the last review.

@Mingela Mingela force-pushed the feature/tests-timeouts branch from 01668ab to f2d9919 Compare November 30, 2018 07:43
@Mingela Mingela merged commit 95bfc13 into develop Nov 30, 2018
@Mingela Mingela deleted the feature/tests-timeouts branch November 30, 2018 08:19
Alexey-N-Chernyshov pushed a commit that referenced this pull request Dec 14, 2018
* Wrapped most of integration tests with awkward JUnit5 timeouts
Alexey-N-Chernyshov pushed a commit that referenced this pull request Dec 14, 2018
* Wrapped most of integration tests with awkward JUnit5 timeouts
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