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

chore: automated electron tests #1883

Merged
merged 7 commits into from
Jun 19, 2024
Merged

chore: automated electron tests #1883

merged 7 commits into from
Jun 19, 2024

Conversation

axi92
Copy link
Contributor

@axi92 axi92 commented Jun 10, 2024

I just figured out that it is possible to run electron tests with xvfb and that xvfb is already present in the ubuntu-latest github runner image.

This little example electron app connects on launch to mqtt://broker.hivemq.com:1883 in the electron web context.

I am yet not sure on how to integrate this into the project.

Todo:

@axi92 axi92 changed the title feat:automated electron tests feat: Automated electron tests Jun 10, 2024
@robertsLando robertsLando changed the title feat: Automated electron tests feat: automated electron tests Jun 11, 2024
@robertsLando robertsLando changed the title feat: automated electron tests chore: automated electron tests Jun 11, 2024
@robertsLando
Copy link
Member

@axi92 Well done with this, does it works? Another thing you could do is to add an export of is-browser things in mqtt.ts or index.ts so you can check for those values too in your test :)

@axi92
Copy link
Contributor Author

axi92 commented Jun 11, 2024

@robertsLando I added the isBrowser test and also check the protocol string from the client that is used for the connection to make sure that it is not falling back to ws

@axi92 axi92 requested a review from robertsLando June 11, 2024 07:51
@GuoSirius
Copy link

In fact, this test should not be placed here for judgment.

Because the rendering process is also a browser environment, since it is a browser environment, it should go the browser logic.

Otherwise, the ws module may be mistakenly loaded in the browser environment of the rendering process, and the ws module cannot run in the browser environment.

If you do need to use the mqtt protocol in an electron application, it is recommended to put the connection establishment logic in the main process, and then use the interprocess communication mechanism to interact between the row rendering process and the main process.

The rendering process must have a window and a document, but not necessarily a Node.js environment, depending on the configuration at instantiation time.

With the main rendering process's responsibilities clearly divided, the browser environment's detection mechanism can maintain the previous standard judgment logic.

@GuoSirius
Copy link

The Electron main process owns all Node environments and can establish connections according to the logic of non-browser environments.

The rendering process belongs to the browser environment and should follow the logic of the browser environment to establish the connection.

Just because the rendering process has more process, Node, and other accessible environments than the normal browser environment, it should not be attributed to the Node environment.

In this cross-end application, it is up to the user to decide which way to establish the connection. According to the needs, the connection can be established in the main process or the rendering process, rather than the application library to force detection to determine the protocol.

According to the above convention, the is-browser.ts file is purely used to detect the browser environment, regardless of whether there are other factors, such as the main process or the rendering process.

@axi92
Copy link
Contributor Author

axi92 commented Jun 18, 2024

I rebased my branch.

@robertsLando robertsLando merged commit 16471a6 into mqttjs:main Jun 19, 2024
4 of 5 checks passed
@axi92 axi92 deleted the feature/automated-electron-tests branch June 19, 2024 20:49
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