-
Notifications
You must be signed in to change notification settings - Fork 23
Fixes skupperproject#1750: Added asan and tsan to the new arm64 runner #1749
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
Fixes skupperproject#1750: Added asan and tsan to the new arm64 runner #1749
Conversation
976331f
to
920066f
Compare
920066f
to
aa7c7f9
Compare
rx.teardown() | ||
clogger.teardown() | ||
|
||
self._wait_address_gone(self.EA1, "balanced/test-address") |
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.
Is there a reason these are removed? The problem is if these addresses are not flushed out before the next test runs then they interfere with the test and things get flakey
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.
The test was waiting on the _wait_address_gone()
and looping inside that function and ultimately timed out. I removed it because test_13_streaming_balanced_parallel()
was the last test in the test suite (StreamingMessageTest
). I understand that removing this code would be a problem if we ever add a test_14
but this test annoyingly keeps failing intermittently in aarch64. I could put it back if you feel strongly about it.
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 makes sense, but it leaves a time bomb for some poor unsuspecting dev like myself when I forget that test_13_xxx must be last. Would you be ok with changing the test number to some obvious indication that it must be last and drop a comment? I know I've used the prefix test_9999_xxx in those other test cases were a test needed to be run last in a test case (see tcp adaptor tests for an example)
|
||
def add_receivers(self): | ||
if len(self.receivers) < self.count: | ||
while len(self.receivers) < self.count: |
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 changes the fundamental behavior of the test. But from looking at the router source code there is no MAX_KEPT_DELTAS appearing anywhere so maybe the functionality this test was intended to exercise is no longer present? @ted-ross does this test ring a bell?
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.
MAX_KEPT_DELTAS is no longer in the router. When the "mobile" functionality moved from Python to C, that feature was removed. MAX_KEPT_DELTAS is now effectively 1.
TopologyAdditionTests.routers.append(new_router) | ||
|
||
TopologyAdditionTests.routers.append(TopologyAdditionTests.tester.qdrouterd(name, config, wait=True)) | ||
if neighbors is not None: |
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.
Very nice!
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.
Your patch LGTM.
self.delay_timer = None | ||
self.count = 2000 | ||
self.count = 1000 | ||
self.delay_count = 12 # This should be larger than MAX_KEPT_DELTAS in mobile.py |
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.
A value of 2 would achieve the desired result.
4a269ae
to
25c1e77
Compare
…erry Pi (#1746)