-
Notifications
You must be signed in to change notification settings - Fork 86
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
Extend bucket notification timeout and map to the test timeout #2135
base: development/2.6
Are you sure you want to change the base?
Changes from 4 commits
a2ec22a
1b2f5f3
0257dca
e22a9de
0125893
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -237,14 +237,15 @@ Then('i {string} be able to add user metadata to object {string}', | |
} | ||
}); | ||
|
||
Then('kafka consumed messages should not take too much place on disk', { timeout: -1 }, | ||
const kafkaCleanerTimeout = 1800000; // 30min | ||
Then('kafka consumed messages should not take too much place on disk', { timeout: kafkaCleanerTimeout }, | ||
async function (this: Zenko) { | ||
const kfkcIntervalSeconds = parseInt(this.parameters.KafkaCleanerInterval); | ||
const checkInterval = kfkcIntervalSeconds * (1000 + 5000); | ||
|
||
const timeoutID = setTimeout(() => { | ||
assert.fail('Kafka cleaner did not clean the topics within the expected time'); | ||
}, checkInterval * 10); // Timeout after 10 Kafka cleaner intervals | ||
}, Math.min(kafkaCleanerTimeout, checkInterval * 10)); // Timeout after 10 Kafka cleaner intervals | ||
|
||
try { | ||
const ignoredTopics = ['dead-letter']; | ||
|
@@ -307,6 +308,9 @@ Then('kafka consumed messages should not take too much place on disk', { timeout | |
|
||
// If a topic remains in this array, it means it has not been cleaned | ||
assert(topics.length === 0, `Topics ${topics.join(', ')} still have not been cleaned`); | ||
} catch (err: unknown) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this not done in cucumber/ctst? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have a "finally" that clears the timeout even if we got an error, which mean we never stop the test as its timeout was -1 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what I mean is that (even with the catch) the exception should be propagated: and eventually reach cucumber/ctst where it shoudl be caught.... and immediately fail the test (even with no timeout) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe the timeout was not due to an error... At least this ensures we log the error with a message Errors not catched by the tests are sent to cucumber directly, it'll mark the test as failed. But in the build above, it didn't work |
||
this.logger.error('Error while checking Kafka cleaner', { err }); | ||
assert.fail(err as Error); | ||
} finally { | ||
clearTimeout(timeoutID); | ||
} | ||
|
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.
30 minutes seems verrrry long for kafka cleaner, which is setup to run every 30 second (or maybe 1-2 minutes) in CI...
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's already 30min today, this was the config where we got the best results
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 am trying this now 0125893