-
Notifications
You must be signed in to change notification settings - Fork 7
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
fix flaky integration tests in xample-domain #2931
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
verified that the CI fails (failure was intentional, per 93cb964) - although wasn't able to get insight into the log file handling, as that step was skipped, due to the failure detected at the integration test step. Will rely on code review for assessing whether the change to log file handling is acceptable. |
rabbitAdmin.purgeQueue(queueName, true); | ||
@BeforeEach | ||
private void setUp() { | ||
rabbitTemplate.setRetryTemplate(retryTemplate); |
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 placement of setRetryTemplate()
is not ideal, for a few reasons:
- it should be executed just once, such as in a
@BeforeAll
function. However, the method signature for@BeforeAll
is static, which is incompatible with therabbitTemplate
instance. - it should be configured in spring (or whatever it is that sets up
rabbitTemplate
)
... and I'm stumped on how to implement either of these. Very open to pointers. Thanks.
private void tearDown() { | ||
rabbitAdmin.purgeQueue(queueName, true); |
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 removed this method as I thought it redundant to the purgeQueue()
performed in setUp()
This comment was marked as outdated.
This comment was marked as outdated.
why: the queue gets purged before each test; the post-test purge seems redundant
(will this help us avoid the 'failed to check/redeclare auto-delete queue(s)' error?)
(for the scope of this PR)
(should have been part of a65133f)
why: i want to see the full CI behavior on this branch; and i'm not ready to move it out of draft mode
(should have been part of cdd0a55)
… retries) initial connection failures for rabbitmq being interpreted as test failures?
although add condition that we will discard those related to AMQP's SimpleMessageListenerContainer
and remove standalone unit test, which itself feels inappropriate (it's testing rabbitMQ functionality, which seems inappropriate for an integratino test
why: verify that it's exposed by the log file handling
the intent had been to catch integration test failures. however, if there are any integration test failures, this step won't be executed.
3dfab3c
to
f04146e
Compare
Great questions in this PR about implementation. I know the static method stuff I've bumped into a lot and don't really have an answer and unfortunately I'm not much of a spring expert either. |
Proceeding with merging. The implementation could be better. On the other hand, it's scoped to a test class that wouldn't affect production; and it is effective with respect to reducing flakiness. Given my limited springboot comfort, I'm not confident in the value of spending more time on this. Forgive me, whoever next handles this code. |
What was the problem?
The continuous Integration (CI) tests for
xample-domain
are flaky: they appear to fail randomly for no apparent reason.As reported in #2345, this error is consistent in the logs of those failed tests:
Theory: 1) the
domain-xample
integration test code (java) occasionally fails to connect to the rabbitMQ service, even if the service is observed to be running*; 2) initial connection failures are not uncommon; and 3) that if given a modest number of retry attempts, the connection would succeed.Associated tickets or Slack threads:
How does this fix it?
To mitigate the previous item being too lax, assess an integration failure if the word "Test failures:" is found in the log file.. (test failures will in fact cause the the log file assessment to be skipped)This PR has an additional change to the integration test, I think minor (removal of an action I thought redundant).
How to test this PR
*the CI workflow verifies the rabbitMQ service is ready before moving on to executing the integration tests.
abd-vro/.github/workflows/xample-integration-test.yml
Lines 46 to 50 in e3ec82a
Footnotes
To check if a PR will succeed in the SecRel workflow, test PRs in the SecRel pipeline. ↩