fix(test): close open connections

[MaximoLiberata]

Improve the logic of close open connections when you instance a serverBuilder class or open a new connection using client. Call method assert interrupts the test and the method after will never called generating that the connections haven't closed yet will still open and other unit test will throw a error like Error: listen EADDRINUSE: address already in use.

This PR also remove the temp folders and files generated by this test when failed.

[MaximoLiberata]

@robertsLando the last test we need to fix to run all test as success all the time is this test. After executing thousand times the test mentioned, sometimes it will fail with always the same error below:

✖ should not overtake the messages stored in the level-db-store
      { message: "expected 'payload3' to equal 'payload2'", showDiff: true, actual: 'payload3', expected: 'payload2', operator: 'strictEqual' }

Does it necessary to prioritize the order in this test?

[robertsLando]

I also see many times that a test_store folder remains created and empty after failed tests and it could be because of that... my feel is that for some reasons maybe the store folder is the same as the test are spawned in parallel and this creates the error? Maybe adding a more random test folder name could mitigate the issue?

Also I think the third publish should be put inside the payload2 publish callback to be 100% sure that the two are sent consecutively

[MaximoLiberata]

Creating unique test folder's names for parallel processes will mitigate the issues related of using resource shared, example: in case of a folder was removed before finishing a test that was using the same folder or file. But the test is using fs.mkdtempSync and this method creates unique temp folder's names. The test will never generate an issue of remaining files for this reason.

I trust more in the correlational issues:

1. If one test fails the following tests will hang or fail, too.
2. If one test didn't successfully close the opened connections like client or server, then they will still open and the following tests will hang or fail because they use the same client or server (maybe the same port).

Right now the after method only will called when the test finish successfully, otherwise it will never called. To avoid correlational issues I prefer to use both beforeEach and after in the top of describe scope, then before each test wait for closing opened connections and deleting temp files of the other tests.

Also, I don't know if the test needs to work in this way publish3 and publish2 separated, and not publish3 inside publish2. Can I move it without affect the logic?

[robertsLando]

Right now the after method only will called when the test finish successfully, otherwise it will never called

I didn't know about that, are you 100% sure of this? I dunno if this should be considered a bug or not on nodejs test suite, I would expext after to be called always after a test even when it fails as it is used to cleanup resources. I should check how others work (like jest, mocha, ava etc..)

Also, I don't know if the test needs to work in this way publish3 and publish2 separated, and not publish3 inside publish2. Can I move it without affect the logic?

I didn't wrote that test so sincerly I'm not 100% sure about what was the reason to write it (expecially because I think it should be placed in mqtt-leveldb store package and not here) anyway I would say that it would be more correct and deterministic to put the publish 3 inside publish 2 callback, maybe that's the only missing piece here

[MaximoLiberata]

I didn't know about that, are you 100% sure of this? I dunno if this should be considered a bug or not on nodejs test suite, I would expext after to be called always after a test even when it fails as it is used to cleanup resources. I should check how others work (like jest, mocha, ava etc..)

Yes, but with a trick that I could find. The after method works in normal cases, but in async operations it will never called. The following script is a replication of the issue and the same issue that present this test

const fs = require('node:fs')
const { describe, it } = require('node:test')
const { assert } = require('chai') // Used in abstract_client.ts


describe("test", () => {

    it ("should 1=1", (t, done) => {
        t.after(async () => {
            console.log('\n----- AFTER MATHOD FIRED -----\n')

            await new Promise((resolve) => {
                fs.rm(storePath, { recursive: true }, () => {
                    resolve()
                })
            })
        })

        const storePath = fs.mkdtempSync('testing_store_')
        fs.appendFile(storePath + '/test.txt', 'Hello world', () => {
            // Remove this line to pass the test and the temp folder will be removed.
            // It fails, then the temp folder will not be removed
            assert.equal(2, 1)
            done()
        })

    })

})

I would say that it would be more correct and deterministic to put the publish 3 inside publish 2 callback, maybe that's the only missing piece here

I think the same, but if there are more tests with a similar logic in their execution like this test we take the risk of if one test fail we will have a correlational issue.

[robertsLando]

Yes, but with a trick that I could find. The after method works in normal cases, but in async operations it will never called. The following script is a replication of the issue and the same issue that present this test

I think it could be worth try to open an issue on NodeJS repo. cc @mcollina is this something expected?

I think the same, but if there are more tests with a similar logic in their execution like this test we take the risk of if one test fail we will have a correlational issue.

I know but I don't see so many alternatives right now. I think our best bet is to fix one by one as we see errors like what you are doing now

[mcollina]

This might be a bug in mocha. I don't have much time to investigate minus saying... don't mix promises and callbacks ;).

[robertsLando]

@mcollina We dropped mocha almost one here ago here, do you remember?

We are using node:test module now that's why I was asking you if you are aware of issues with it related to after inside async tests

[robertsLando]

@MaximoLiberata Is it just an impression or tests are failing more frequently now then before?

[MaximoLiberata]

reference test

Also I think the third publish should be put inside the payload2 publish callback to be 100% sure that the two are sent consecutively

I moved publish3 into publish2, now the tests always pass and the tests that I modified in this PR will not generate correlational issues if one of them fail. The rest tests might generate correlational issues.

[MaximoLiberata]

@MaximoLiberata Is it just an impression or tests are failing more frequently now then before?

@robertsLando Could you give more detail about that? they should not fail

[robertsLando]

See https://github.com/mqttjs/MQTT.js/actions/runs/10182861315/job/28166435859?pr=1911

[MaximoLiberata]

See https://github.com/mqttjs/MQTT.js/actions/runs/10182861315/job/28166435859?pr=1911

I didn't clean the references of the objects client and server after each test.

[MaximoLiberata]

Now I pass the references of the objects client and server using cleanMethod.setClient(client) and cleanMethod.setServer(server) respectively. These references will be used by the method cleanMethod.closeClientAndServer() to close their connection.

But I have a question, those reference can't be used before instance client or server because their value start in null or undefined and the CleanMethod class will not track the changes of them, so if there is an internal error in their initialization the CleanMethod class will not have reference of their new values and will not close those connections.

Can class track external change (not inside class by its method) of their attributes with other implementation or not?

[cjihrig]

@MaximoLiberata you found a legit bug in the Node test runner - nodejs/node#54151.

[robertsLando]

Can class track external change (not inside class by its method) of their attributes with other implementation or not?

I'm not sure I understood what you mean here, what attribute would you like to watch of client and server ?

[robertsLando]

@MaximoLiberata you found a legit bug in the Node test runner - nodejs/node#54151.

Thanks @cjihrig, and thanks to @mcollina as I think he has asked you to look at this :)

[robertsLando]

@MaximoLiberata I'm doing some changes, hold a bit

[robertsLando]

@MaximoLiberata Done, check them out then I think we can merge this :)

[robertsLando]

arggg: https://github.com/mqttjs/MQTT.js/actions/runs/10200588659/job/28220438706?pr=1911#step:6:337

[MaximoLiberata]

@robertsLando I just adjust the class' documentation and thank for checking and improve the code. Also, Thank @mcollina and @cjihrig for reporting the bug and make a PR to fix it.

[MaximoLiberata]

arggg: https://github.com/mqttjs/MQTT.js/actions/runs/10200588659/job/28220438706?pr=1911#step:6:337

I think some opened connection never close (waiting for the callback) because the test runs very fast and we are call client.end or server.close as soon as we can do it.

[robertsLando]

I think some opened connection never close (waiting for the callback) because the test run very fast and we are call client.end or server.close as soon as we can do it.

Yeah I know, seems now it get stucks on keeepalive tests. Anyway for sure now it's better then before, would you like me to merge this or do you want to try something else?

[MaximoLiberata]

Yeah I know, seems now it get stucks on keeepalive tests. Anyway for sure now it's better then before, would you like me to merge this or do you want to try something else?

I can try to cover the functions connect and serverBuilder with teardownHelper. In this way we ensure that any instance of them can close. Example:

function connect(opts?: IClientOptions | string) {
	if (typeof opts === 'string') {
		opts = { host: opts }
	}
	opts = { ...config, ...opts } as IClientOptions
	const instance = mqtt.connect(opts)
	teardownHelper.addClient(instance)
	return instance
}

Also, I was trying to execute beforeEach(beforeEachExec) in the top of each describe hook and I found an issue with the execution of some tests according to run them very fast and a possible hang.

// This test is executed very fast before close the client connection
it('should send a subscribe message (offline)', function _test(t, done) {
	const client = connect()

	client.subscribe('test')

	server.once('client', (serverClient) => {
		serverClient.once('subscribe', () => {
			// Change to `client.end(done)` to fix it
			done()
		})
	})
})

[MaximoLiberata]

This change will modify the logic of the initial PR a little bit, so I don't know if adding it in this PR or another.

[robertsLando]

@MaximoLiberata Let's merge this IMO and do the changes on another 👍🏼