Skip to content
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

fix: tests that relied on order of events #1534

Merged
merged 12 commits into from
Jul 6, 2023
Merged

fix: tests that relied on order of events #1534

Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d41495c
fix tests that relied on order of events
BertKleewein Sep 8, 2022
8da5c6f
all tests passing
BertKleewein Sep 9, 2022
05e264c
Merge branch 'main' into fix-tests
robertsLando Jun 27, 2023
cbea24f
Merge branch 'main' into fix-tests
robertsLando Jun 27, 2023
a47c2e7
Merge branch 'main' into fix-tests
robertsLando Jun 28, 2023
6d50218
Merge branch 'main' into fix-tests
robertsLando Jun 30, 2023
8ce5746
fix client and server cleanup to make tests less flaky
Jul 5, 2023
3e8d661
more changes to test case cleanup
Jul 5, 2023
d31cb5f
remove disconnect calls. This doesn't disconnect, it just sends a pa…
Jul 5, 2023
a6a770a
roll back timeout changes
Jul 5, 2023
dfee75d
add comments
Jul 6, 2023
c08c7c3
Merge branch 'main' into fix-tests
BertKleewein Jul 6, 2023
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
99 changes: 70 additions & 29 deletions test/abstract_client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,12 +235,12 @@ module.exports = function (server, config) {

describe('connecting', function () {
it('should connect to the broker', function (done) {
this.timeout(10000)
const client = connect()
client.on('error', done)

server.once('client', function () {
done()
client.end()
client.end(done)
})
})

Expand Down Expand Up @@ -550,6 +550,7 @@ module.exports = function (server, config) {
})

describe('offline messages', function () {
this.timeout(20000)
it('should queue message until connected', function (done) {
const client = connect()

Expand Down Expand Up @@ -733,6 +734,7 @@ module.exports = function (server, config) {
})

describe('publishing', function () {
this.timeout(10000)
it('should publish a message (offline)', function (done) {
const client = connect()
const payload = 'test'
Expand Down Expand Up @@ -833,9 +835,15 @@ module.exports = function (server, config) {
retain: true,
qos: 1
}
let received = false

client.once('connect', function () {
client.publish(topic, payload, opts)
client.publish(topic, payload, opts, function (err) {
assert(received)
client.end(function () {
done(err)
})
})
})

server.once('client', function (serverClient) {
Expand All @@ -845,7 +853,7 @@ module.exports = function (server, config) {
assert.strictEqual(packet.qos, opts.qos, 'incorrect qos')
assert.strictEqual(packet.retain, opts.retain, 'incorrect ret')
assert.strictEqual(packet.dup, false, 'incorrect dup')
client.end(done)
received = true
})
})
})
Expand All @@ -872,7 +880,7 @@ module.exports = function (server, config) {
})
})

it('should mark a message as duplicate when "dup" option is set', function (done) {
it('should mark a message as duplicate when "dup" option is set', function (done) {
const client = connect()
const payload = 'duplicated-test'
const topic = 'test'
Expand All @@ -881,9 +889,15 @@ module.exports = function (server, config) {
qos: 1,
dup: true
}
let received = false

client.once('connect', function () {
client.publish(topic, payload, opts)
client.publish(topic, payload, opts, function (err) {
assert(received)
client.end(function () {
done(err)
})
})
})

server.once('client', function (serverClient) {
Expand All @@ -893,7 +907,7 @@ module.exports = function (server, config) {
assert.strictEqual(packet.qos, opts.qos, 'incorrect qos')
assert.strictEqual(packet.retain, opts.retain, 'incorrect ret')
assert.strictEqual(packet.dup, opts.dup, 'incorrect dup')
client.end(done)
received = true
})
})
})
Expand Down Expand Up @@ -957,19 +971,29 @@ module.exports = function (server, config) {

it('should publish 10 QoS 2 and receive them', function (done) {
const client = connect()
let count = 0
let countSent = 0
let countReceived = 0

function publishNext () {
client.publish('test', 'test', { qos: 2 }, function (err) {
assert.ifError(err)
countSent++
})
}

client.on('connect', function () {
client.subscribe('test')
client.publish('test', 'test', { qos: 2 })
client.subscribe('test', function (err) {
assert.ifError(err)
publishNext()
})
})

client.on('message', function () {
if (count >= 10) {
client.end()
done()
countReceived++
if (countSent >= 10 && countReceived >= 10) {
client.end(done)
} else {
client.publish('test', 'test', { qos: 2 })
publishNext()
}
})

Expand All @@ -984,10 +1008,6 @@ module.exports = function (server, config) {
serverClient.publish(packet)
})
})

serverClient.on('pubrel', function () {
count++
})
})
})

Expand Down Expand Up @@ -1418,29 +1438,41 @@ module.exports = function (server, config) {
describe('unsubscribing', function () {
it('should send an unsubscribe packet (offline)', function (done) {
const client = connect()
let received = false

client.unsubscribe('test')
client.unsubscribe('test', function (err) {
assert.ifError(err)
assert(received)
client.end(done)
})

server.once('client', function (serverClient) {
serverClient.once('unsubscribe', function (packet) {
assert.include(packet.unsubscriptions, 'test')
client.end(done)
received = true
})
})
})

it('should send an unsubscribe packet', function (done) {
const client = connect()
const topic = 'topic'
let received = false

client.once('connect', function () {
client.unsubscribe(topic)
client.unsubscribe(topic, function (err) {
assert.ifError(err)
assert(received)
client.end(done)
})
})

// TODO: all of these test changes have to do with calling end() before the callback is complete. Maybe that should be fixed first
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depends on what you mean by "this" :) Do you mean "make these tests better" or do you mean "fix the problems that happen when you call client.end from inside a callback?"

IMHO, one of the ugliest problems in this code (MQTT.js), is the spaghetti flow that happens when you call client.end. I've tried to make sense of the order that things get shut down, and there are too many branches and patches and callbacks inside that code to make me confident in any changes to the shutdown code.

If the flow of client.end does get fixed, it needs to be done very carefully. I assume that customers have worked around its oddities (whether they know it or not), and it just really feels like changing how client.end works is opening a large can of worms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I forgot to say that I'm assuming that you want to make these tests better. Sure, I can do that.

Copy link
Member

Choose a reason for hiding this comment

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

Depends on what you mean by "this" :) Do you mean "make these tests better" or do you mean "fix the problems that happen when you call client.end from inside a callback?"

Sorry for not being clear. I assumed both, anyway I already checked the end() code and it's a real mess so we can keep that for another PR 🙏🏼


server.once('client', function (serverClient) {
serverClient.once('unsubscribe', function (packet) {
assert.include(packet.unsubscriptions, topic)
client.end(done)
received = true
})
})
})
Expand Down Expand Up @@ -1478,15 +1510,20 @@ module.exports = function (server, config) {
it('should accept an array of unsubs', function (done) {
const client = connect()
const topics = ['topic1', 'topic2']
let received = false

client.once('connect', function () {
client.unsubscribe(topics)
client.unsubscribe(topics, function (err) {
assert.ifError(err)
assert(received)
client.end(done)
})
})

server.once('client', function (serverClient) {
serverClient.once('unsubscribe', function (packet) {
assert.deepStrictEqual(packet.unsubscriptions, topics)
client.end(done)
received = true
})
})
})
Expand Down Expand Up @@ -2462,7 +2499,7 @@ module.exports = function (server, config) {
})

it('should resend in-flight QoS 1 publish messages from the client', function (done) {
this.timeout(4000)
this.timeout(10000)
const client = connect({ reconnectPeriod: 200 })
let serverPublished = false
let clientCalledBack = false
Expand Down Expand Up @@ -2954,10 +2991,9 @@ module.exports = function (server, config) {
serverClient.pubrec({ messageId: packet.messageId })
}
})
serverClient.on('pubrel', function () {
serverClient.on('pubrel', function (packet) {
if (reconnect) {
server2.close()
client.end(true, done)
serverClient.pubcomp({ messageId: packet.messageId })
} else {
client.end(true, function () {
client.reconnect({
Expand All @@ -2983,7 +3019,12 @@ module.exports = function (server, config) {

client.on('connect', function () {
if (!reconnect) {
client.publish('topic', 'payload', { qos: 2 })
client.publish('topic', 'payload', { qos: 2 }, function (err) {
server2.close()
Copy link
Member

Choose a reason for hiding this comment

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

Does server.close expects a callback too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like it does. Do you think I should update the tests to use it? I assume it will help, but I can't guarantee that it won't cause other problems.

Copy link
Member

Choose a reason for hiding this comment

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

Looks better now :)

assert(reconnect)
assert.ifError(err)
client.end(true, done)
})
}
})
client.on('error', function () {})
Expand Down