-
Notifications
You must be signed in to change notification settings - Fork 111
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
[DATA-749] Fix SLAM integration test timeout #1600
[DATA-749] Fix SLAM integration test timeout #1600
Conversation
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.
LGTM, just a few nits
if !orbslam_hangs { | ||
test.That(t, err, test.ShouldBeNil) | ||
} else if err != nil { | ||
t.Skip("Skipping test because orbslam failed to shut down") |
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.
[nit] log should mention the hang issue
@@ -293,13 +335,27 @@ func integrationTestHelper(t *testing.T, mode slam.Mode) { | |||
break | |||
} | |||
test.That(t, strings.Contains(line, "Fail to track local map!"), test.ShouldBeFalse) | |||
if time.Since(start_time_sent_image) > time.Duration(dataInsertionMaxTimeoutMin)*time.Minute { | |||
orbslam_hangs = true | |||
t.Log("orbslam hangs") |
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.
[nit] log should be more descriptive, maybe mention exiting data loop?
@@ -153,13 +159,27 @@ func integrationTestHelper(t *testing.T, mode slam.Mode) { | |||
break | |||
} | |||
test.That(t, strings.Contains(line, "Fail to track local map!"), test.ShouldBeFalse) | |||
if time.Since(start_time_sent_image) > time.Duration(dataInsertionMaxTimeoutMin)*time.Minute { | |||
orbslam_hangs = true | |||
t.Log("orbslam hangs") |
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.
[nit] log should be more descriptive, maybe mention exiting data loop?
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.
Great check to limit/control time allowed for images to be passed. Suggested some reordering of the for loop to prevent hanging log from appearing and then a successful pass.
|
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.
LGTM!
https://viam.atlassian.net/browse/DATA-749
Orbslam is known to hang frequently indefinitely, especially when running it with a mono camera. When orbslam hangs, it will cause a timeout and is the most likely suspect for the observed timeout issues mentioned in the ticket.
This PR updates the integration test to make sure adding an image to orbslam does not exceed 3 minutes. If it does, we assume that orbslam hangs and handle it accordingly.