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

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

Merged
merged 6 commits into from Dec 3, 2021
Merged

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

merged 6 commits into from Dec 3, 2021

Conversation

ghost
Copy link

@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?

@lbry-bot lbry-bot assigned eukreign and unassigned eukreign Nov 24, 2021
@eukreign
Copy link
Member

@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
Copy link
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

coveralls commented Nov 30, 2021

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
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
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
Labels
area: tests type: improvement Existing (or partially existing) functionality needs to be changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants