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.

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

feat(electron-test): move third hosted broker to self hosted broker #1926

Merged
merged 7 commits into from
Aug 30, 2024

Conversation

MaximoLiberata
Copy link
Contributor

electron tests may present connection issues when try to connect to third hosted broker. The tests will fail for external issues not related to MQTT.JS. A self hosted broker will mitigate these issues.

@MaximoLiberata
Copy link
Contributor Author

@axi92 Could you review this PR, too?

Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

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

I higly suggest to use aedes-cli instead:

await start({
protos: ['tcp', 'tls', 'ws', 'wss'],
wsPort,
wssPort,
key: './test/certs/server-key.pem',
cert: './test/certs/server-cert.pem',
verbose: true,
stats: false
})

@MaximoLiberata
Copy link
Contributor Author

MaximoLiberata commented Aug 9, 2024

@robertsLando I have a question, the ServerLauncher class/service is launched before each test starts. So far, the electron tests didn't fail, but I'm not sure if they will fail because duplicate server ports like node test issues.

Is there a way to close aedes-cli broker? I saw its documentation, but I didn't find a method to close it

@robertsLando
Copy link
Member

@MaximoLiberata start returns servers, broker instances so yeah you can close them:

https://github.com/moscajs/aedes-cli/blob/master/lib/cli.js#L420

@MaximoLiberata
Copy link
Contributor Author

Thanks @robertsLando

electron-test/tsconfig.json Outdated Show resolved Hide resolved
electron-test/wdio.conf.ts Outdated Show resolved Hide resolved
electron-test/tsconfig.json Outdated Show resolved Hide resolved
@robertsLando
Copy link
Member

@MaximoLiberata Sorry for the late review but I'm on holiday, I will be back on the end of August :)

electron-test/wdio.conf.ts Outdated Show resolved Hide resolved
Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

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

Just came back from holidays, sorry for the late review! Everything looks good to me, just remove the useless option and I will merge this, thanks 🙏🏼

electron-test/wdio.conf.ts Outdated Show resolved Hide resolved
@MaximoLiberata
Copy link
Contributor Author

thanks @robertsLando and @axi92 for review 🙏🏼

@robertsLando robertsLando merged commit 1ca3f9e into mqttjs:main Aug 30, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants