Skip to content

Added timeout of async operations to integration test setup/teardown#3490

Merged
eukreign merged 6 commits intomasterfrom
unknown repository
Dec 3, 2021
Merged

Added timeout of async operations to integration test setup/teardown#3490
eukreign merged 6 commits intomasterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Nov 20, 2021

pull request addressing the issue #3482
Had a look at the causes of the 6h pipeline runs (such as this) - appeared to be async operations in the test cleanup procedure. This change adds timeout to the setup and cleanup of the integration tests.

Is it known why the operations were getting stuck? If not - could more logging be added to look into this problem further?

@lyoshenka lyoshenka requested a review from eukreign November 24, 2021 16:04
@lbry-bot lbry-bot assigned eukreign and unassigned eukreign Nov 24, 2021
@eukreign
Copy link
Copy Markdown
Member

eukreign commented Nov 29, 2021

@femtosecondlaser thanks for this PR!

the tests don't seem to be passing... also, where does TIMEOUT get set?

i think we always want all tests to have a timeout, there should be some default timeout set (how about 30 seconds)?

@eukreign eukreign assigned ghost and unassigned eukreign Nov 29, 2021
eukreign
eukreign requested changes Nov 29, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@eukreign eukreign left a comment

Choose a reason for hiding this comment

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

TIMEOUT should be set to a default value somewhere and we always want the timeout enabled, unless you can think of a reason a test might not want to have a timeout?

@coveralls
Copy link
Copy Markdown

coveralls commented Nov 30, 2021 •
edited
Loading

Coverage Status

Coverage decreased (-0.05%) to 69.636% when pulling e4777f9 on FemtosecondLaser:integration_test_setup_cleanup_timeouts into 3508f56 on lbryio:master.

@ghost
Copy link
Copy Markdown
Author

ghost commented Nov 30, 2021

TIMEOUT should be set to a default value somewhere and we always want the timeout enabled, unless you can think of a reason a test might not want to have a timeout?

TIMEOUT is set to 120 sec here. It has been there for timing out async operations in the main body of the tests. I extended the usage for startup and cleanup parts of the tests.
Timeout gets disabled for some tests here. The removal of conditional set up of timeout makes those tests unhappy.

@ghost ghost requested a review from eukreign November 30, 2021 01:41
@ghost
Copy link
Copy Markdown
Author

ghost commented Nov 30, 2021

@eukreign
I suspect the tests are failing for reasons unrelated to this change.
The latest run failed while building for mac. All the tests passed.
I don't have an option to re-run the pipe, could you do that please.

@eukreign eukreign added area: tests type: improvement Existing (or partially existing) functionality needs to be changed labels Dec 1, 2021
@eukreign eukreign merged commit 6343771 into lbryio:master Dec 3, 2021
@jackrobison jackrobison changed the title added timeout of async operations to integration test setup/teardown Added timeout of async operations to integration test setup/teardown Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

area: tests type: improvement Existing (or partially existing) functionality needs to be changed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@eukreign @coveralls