Skip to content

Fix flake on rabbitmq_mqtt auth_SUITE #13180

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Feb 12, 2025
Merged

Fix flake on rabbitmq_mqtt auth_SUITE #13180

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d689a16
Separate invalid client test from the valid one
MarcialRosales Jan 30, 2025
de8f59d
Apply same changes from pr #13197
MarcialRosales Feb 5, 2025
3a61274
Deal with stalereferences caused by timing issues
MarcialRosales Feb 5, 2025
3ca11fc
Unlink before assertion
MarcialRosales Feb 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions deps/rabbitmq_mqtt/test/auth_SUITE.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,11 @@ sub_groups() ->
ssl_user_vhost_parameter_mapping_vhost_does_not_exist,
ssl_user_cert_vhost_mapping_takes_precedence_over_port_vhost_mapping
]},
{ssl_user_with_invalid_client_id_in_cert_san_dns, [],
[invalid_client_id_from_cert_san_dns
]},
{ssl_user_with_client_id_in_cert_san_dns, [],
[client_id_from_cert_san_dns,
invalid_client_id_from_cert_san_dns
[client_id_from_cert_san_dns
]},
{ssl_user_with_client_id_in_cert_san_dns_1, [],
[client_id_from_cert_san_dns_1
Expand Down Expand Up @@ -207,7 +209,8 @@ mqtt_config(no_ssl_user) ->
mqtt_config(client_id_propagation) ->
{rabbitmq_mqtt, [{ssl_cert_login, true},
{allow_anonymous, true}]};
mqtt_config(ssl_user_with_client_id_in_cert_san_dns) ->
mqtt_config(T) when T == ssl_user_with_client_id_in_cert_san_dns;
T == ssl_user_with_invalid_client_id_in_cert_san_dns ->
{rabbitmq_mqtt, [{ssl_cert_login, true},
{allow_anonymous, false},
{ssl_cert_client_id_from, subject_alternative_name},
Expand Down Expand Up @@ -588,7 +591,7 @@ client_id_from_cert_dn(Config) ->
invalid_client_id_from_cert_san_dns(Config) ->
MqttClientId = <<"other_client_id">>,
{ok, C} = connect_ssl(MqttClientId, Config),
?assertMatch({error, _}, emqtt:connect(C)),
{error, {client_identifier_not_valid, _}} = emqtt:connect(C),
Copy link
Member

@ansd ansd Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR doesn't fix the flake. I can repro the flake locally as follows:

    {error, {client_identifier_not_valid, _}} = emqtt:connect(C),
    timer:sleep(1),
    unlink(C).

which makes the test fail with:

=== === Reason: {'EXIT',{shutdown,client_identifier_not_valid}}

That's the flake which we also observed in CI.

The correct fix is to unlink first, then connect.

Copy link
Contributor Author

@MarcialRosales MarcialRosales Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i cannot reproduce it locally and i dont see CI failing because of this . I can modify the test as you suggest though.

Copy link
Contributor Author

@MarcialRosales MarcialRosales Feb 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still dont understand what you are suggesting. I am expecting that when the user connects it fails because the client_id is not valid. That is exactly what it is doing line 594.

Copy link
Contributor Author

@MarcialRosales MarcialRosales Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you send me the link to the failed job in CI where this test case is failing ?

Copy link
Member

@ansd ansd Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you send me the link to the failed job in CI where this test case is failing ?

Yes, here are two flakes failing due to {'EXIT',{shutdown,client_identifier_not_valid}}:

Copy link
Member

@ansd ansd Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i cannot reproduce it locally and i dont see CI failing because of this .

The CT test case process runs concurrently with the MQTT connection process. CI can fail if the CT test case process executes unlink(C) after receiving the EXIT signal.

Copy link
Member

@ansd ansd Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still dont understand what you are suggesting.

I'm suggesting to flip the two lines, i.e. to unlink before the expected failure as done in many other places in this test suite, e.g. in:

Copy link
Contributor Author

@MarcialRosales MarcialRosales Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those ci failures are because this change has not been merged yet

Copy link
Contributor Author

@MarcialRosales MarcialRosales Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless i have committed that change.

Copy link
Contributor Author

@MarcialRosales MarcialRosales Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but in this PR, with just my changes you dont see the flake. You will see the flake in other CI which do not have this fix.

unlink(C).

ssl_user_vhost_parameter_mapping_success(Config) ->
Expand Down
2 changes: 1 addition & 1 deletion selenium/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
"author": "",
"license": "ISC",
"dependencies": {
"chromedriver": "^130.0.4",
"chromedriver": "^132.0",
"ejs": "^3.1.8",
"express": "^4.18.2",
"geckodriver": "^3.0.2",
Expand Down
88 changes: 60 additions & 28 deletions selenium/test/pageobjects/BasePage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ module.exports = class BasePage {
return this.selectOption(SELECT_REFRESH, option)
}
async waitForOverviewTab() {
await this.driver.sleep(250)
return this.waitForDisplayed(OVERVIEW_TAB)
}

Expand All @@ -56,27 +57,31 @@ module.exports = class BasePage {
return this.click(CONNECTIONS_TAB)
}
async waitForConnectionsTab() {
await this.driver.sleep(250)
return this.waitForDisplayed(CONNECTIONS_TAB)
}

async clickOnAdminTab () {
return this.click(ADMIN_TAB)
}
async waitForAdminTab() {
await this.driver.sleep(250)
return this.waitForDisplayed(ADMIN_TAB)
}

async clickOnChannelsTab () {
return this.click(CHANNELS_TAB)
}
async waitForChannelsTab() {
await this.driver.sleep(250)
return this.waitForDisplayed(CHANNELS_TAB)
}

async clickOnExchangesTab () {
return this.click(EXCHANGES_TAB)
}
async waitForExchangesTab() {
await this.driver.sleep(250)
return this.waitForDisplayed(EXCHANGES_TAB)
}

Expand Down Expand Up @@ -180,42 +185,69 @@ module.exports = class BasePage {
}

async waitForLocated (locator) {
try {
return this.driver.wait(until.elementLocated(locator), this.timeout,
'Timed out after [timeout=' + this.timeout + ';polling=' + this.polling + '] seconds locating ' + locator,
this.polling)
}catch(error) {
if (!error.name.includes("NoSuchSessionError")) {
console.error("Failed waitForLocated " + locator + " due to " + error)
}
throw error
}
let attempts = 3
let retry = false
let rethrowError = null
do {
try {
return this.driver.wait(until.elementLocated(locator), this.timeout,
'Timed out after [timeout=' + this.timeout + ';polling=' + this.polling + '] seconds locating ' + locator,
this.polling)
}catch(error) {
if (error.name.includes("StaleElementReferenceError")) {
retry = true
}else if (!error.name.includes("NoSuchSessionError")) {
console.error("Failed waitForLocated " + locator + " due to " + error)
retry = false
}
rethrowError = error
}
} while (retry && --attempts > 0)
throw rethrowError
}

async waitForVisible (element) {
try {
return this.driver.wait(until.elementIsVisible(element), this.timeout,
'Timed out after [timeout=' + this.timeout + ';polling=' + this.polling + '] awaiting till visible ' + element,
this.polling)
}catch(error) {
if (!error.name.includes("NoSuchSessionError")) {
console.error("Failed to find visible element " + element + " due to " + error)
let attempts = 3
let retry = false
let rethrowError = null
do {
try {
return this.driver.wait(until.elementIsVisible(element), this.timeout,
'Timed out after [timeout=' + this.timeout + ';polling=' + this.polling + '] awaiting till visible ' + element,
this.polling)
}catch(error) {
if (error.name.includes("StaleElementReferenceError")) {
retry = true
}else if (!error.name.includes("NoSuchSessionError")) {
console.error("Failed to find visible element " + element + " due to " + error)
retry = false
}
rethrowError = error
}
throw error
}
} while (retry && --attempts > 0)
throw rethrowError
}


async waitForDisplayed (locator) {
if (this.interactionDelay && this.interactionDelay > 0) await this.driver.sleep(this.interactionDelay)
try {
return this.waitForVisible(await this.waitForLocated(locator))
}catch(error) {
if (!error.name.includes("NoSuchSessionError")) {
console.error("Failed to waitForDisplayed " + locator + " due to " + error)
}
throw error
}
let attempts = 3
let retry = false
let rethrowError = null
do {
if (this.interactionDelay && this.interactionDelay > 0) await this.driver.sleep(this.interactionDelay)
try {
return this.waitForVisible(await this.waitForLocated(locator))
}catch(error) {
if (error.name.includes("StaleElementReferenceError")) {
retry = true
}else if (!error.name.includes("NoSuchSessionError")) {
retry = false
console.error("Failed to waitForDisplayed " + locator + " due to " + error)
}
rethrowError = error
}
} while (retry && --attempts > 0 )
throw rethrowError
}

async getText (locator) {
Expand Down
Loading