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

Add testing setup #4

Closed
kevinramharak opened this issue Feb 19, 2024 · 4 comments · Fixed by #5
Closed

Add testing setup #4

kevinramharak opened this issue Feb 19, 2024 · 4 comments · Fixed by #5

Comments

@kevinramharak
Copy link
Contributor

kevinramharak commented Feb 19, 2024

I messed around with the repository to try and add a testing framework.
I initially tried to setup vitest but ran into issues not being able to mock require calls, which causes some issues internally with Phaser when ran in a node environment.

Jest

I tried again with jest and you can check out the branch here: https://github.com/laineus/phavuer/compare/master...kevinramharak:phavuer:feature/add-tests-jest?expand=1.

It became larger than I intended because of the following issues:

Note that the test emits the boot event fails. This is because Phaser only emits that event async when the new Phaser() constructor runs before DOMContentLoaded emits. If it runs after, it will emit the boot event sync (https://github.com/phaserjs/phaser/blob/master/src/dom/DOMContentLoaded.js#L27).
Therefore in Phavuer the lines:

<script>
      const game = new Phaser.Game(Object.assign({ parent: canvasRoot.value }, props.config))
      game.events.addListener('boot', () => {
        context.emit('boot', game)
      })
</script>

Will never actually catch the boot event, because the event is emitted before the listener is attached. Since Phavuer is part of a Vue application, I doubt there will ever be an instance where the vue application is mounted before the DOMContentLoaded event is emitted.
My suggestion would be to drop the boot event. An alternative is to use the postBoot callback (see https://newdocs.phaser.io/docs/3.60.0/Phaser.Types.Core.CallbacksConfig).
But in that case the boot event will actually be emitted after the ready event. see https://github.com/phaserjs/phaser/blob/v3.70.0/src/core/Game.js#L416-L434.

Could you check out the branch and see if this is a workable setup for tests? I might try another vitest setup and see if I can avoid upgrading yarn, but I think that might give more issues since it doesn't aim to support require, and Phaser is very dependent on how require works.


Vitest

With a lot of pain I setup vitest here: https://github.com/laineus/phavuer/compare/master...kevinramharak:phavuer:feature/add-tests-vitest?expand=1

This uses the experimental browser feature from vitest, which has your tests run in an actual browser.
This fixes so much pain with jsdom and canvas, so I think its miles better than using jest with a jsdom/canvas setup.

The only ugly thing there was having to set string-width to ^4.2.2 because of ESM issues deep in the dependency tree.

@laineus
Copy link
Owner

laineus commented Feb 20, 2024

@kevinramharak Thank you!
I will check out your branch later.

I might try another vitest setup and see if I can avoid upgrading yarn

It would be great if this could be achieved.

Thank you for the information regarding the boot issue.
I think I would like to remove the boot event.

@kevinramharak
Copy link
Contributor Author

kevinramharak commented Feb 20, 2024

@laineus

I managed to find a setup with vitest:
https://github.com/laineus/phavuer/compare/master...kevinramharak:phavuer:feature/add-tests-vitest?expand=1

This does not require a yarn upgrade. It also removes a lot of pain with running phaser in node, having to use jsdom and node-canvas.

This uses the experimental browser feature from vitest, which has your tests run in an actual browser.
This fixes so much pain with jsdom and canvas, so I think its miles better than using jest with a jsdom/canvas setup.

The only ugly thing there was having to set string-width to ^4.2.2 because of ESM issues deep in the dependency tree.

@laineus
Copy link
Owner

laineus commented Feb 20, 2024

@kevinramharak Great!

One thing I noticed is that there isn't a 'hideBanner' property in the BannerConfig options. Could it be a mistake for 'hidePhaser'?
https://newdocs.phaser.io/docs/3.60.0/Phaser.Types.Core.BannerConfig

Could you then create a Pull Request against master?

@kevinramharak
Copy link
Contributor Author

kevinramharak commented Feb 20, 2024

@laineus I added a PR at #5

Good catch on the hideBanner. Turns out hidePhaser only hides a piece of the text, so the only way to disable the banner is config.banner === false. I adjust the code accordingly.

I also noticed you dropped the boot event thus I removed the test for it.

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 a pull request may close this issue.

2 participants