-
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
RSDK-702: tweak flakiness in gpio input tests #1626
Conversation
@@ -63,7 +63,7 @@ func setup(t *testing.T) *setupResult { | |||
"interrupt1": { | |||
Control: input.ButtonNorth, | |||
Invert: false, | |||
DebounceMs: 0, | |||
DebounceMs: 20, |
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.
Changed from the default 5ms
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.
Probably fine. Though the original intent was to also check that the default was applied, when the actual value was zero (e.g. when not set in json.)
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.
I'll look into adding another test to check that defaults are set in another pr
err := s.b.Digitals["interrupt2"].Tick(s.ctx, true, uint64(time.Now().UnixNano())) | ||
test.That(t, err, test.ShouldBeNil) | ||
time.Sleep(time.Millisecond) |
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.
i'm not sure what value these sleeps served. my understanding for this test was that commands should be issued as fast as possible and ensure that none get debounced.
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.
Was just "best practice" to keep things from being CPU bound. E.g. depending on load and cpu power, this might go a thousand times faster or slower on different runs/machines, and the sleep means it'll be "approximately" the same anywhere. If it works with them removed, I'm fine with them gone though.
s.axisMu.RUnlock() | ||
} | ||
|
||
s.b.Analogs["analog2"].Set(1023) |
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.
this whole test was removed
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.
Why was this whole test removed? Wasn't it doing something useful?
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.
it tested default vs. overriden poll rate. potential flakiness introduced by testing this way wasn't worth it.
@@ -526,46 +515,16 @@ func TestGPIOInput(t *testing.T) { | |||
target = 0 | |||
s.b.Analogs["analog1"].Set(0) | |||
} | |||
testutils.WaitForAssertion(t, func(tb testing.TB) { | |||
testutils.WaitForAssertionWithSleep(t, 5*time.Millisecond, 25, func(tb testing.TB) { |
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.
test every 5ms instead of every 50ms.
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!
@@ -63,7 +63,7 @@ func setup(t *testing.T) *setupResult { | |||
"interrupt1": { | |||
Control: input.ButtonNorth, | |||
Invert: false, | |||
DebounceMs: 0, | |||
DebounceMs: 20, |
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.
Probably fine. Though the original intent was to also check that the default was applied, when the actual value was zero (e.g. when not set in json.)
err := s.b.Digitals["interrupt2"].Tick(s.ctx, true, uint64(time.Now().UnixNano())) | ||
test.That(t, err, test.ShouldBeNil) | ||
time.Sleep(time.Millisecond) |
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.
Was just "best practice" to keep things from being CPU bound. E.g. depending on load and cpu power, this might go a thousand times faster or slower on different runs/machines, and the sleep means it'll be "approximately" the same anywhere. If it works with them removed, I'm fine with them gone though.
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.
I trust @Otterverse's judgment over my own (hence me leaving a comment instead of an approval), but things LGTM!
s.axisMu.RUnlock() | ||
} | ||
|
||
s.b.Analogs["analog2"].Set(1023) |
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.
Why was this whole test removed? Wasn't it doing something useful?
Note: merge base coverage results not available, comparing against closest eee2f24~1=(d9afabf) instead
|
Actual changes to lower the odds of flakes.