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 unit tests for Application.ticker setter #4730

Merged
merged 1 commit into from Mar 5, 2018

Conversation

schipiga
Copy link
Contributor

@schipiga schipiga commented Mar 2, 2018

Hello folks! I like autotests and would like to start to contribute to your project

Copy link
Member

@bigtimebuddy bigtimebuddy left a comment

Choose a reason for hiding this comment

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

Thanks for being willing to help. Definitely could use some help with writing more tests! Glad you're eager to help. Let me know if you'd like to join the PixiJS Slack.

As for this PR, couple of things:

  • Please remove the use of the fat-arrow => with Mocha, it actually loses the global context, which we'd like to retain for handling variables and other useful bits (like setting timeouts, slow, etc). As a practice, we keep that in using function.
  • Not sure beforeEach is a good choice here. Even though that the autoStart test is the exception, in the future creating with a variety of constructor options is desirable. Would get messy with beforeEach.
  • I would just add a before in your describe and set this.app = new PIXI.Application(), then reference this.app in your ticker tests. Make sure to destroy it with after.

@bigtimebuddy bigtimebuddy changed the title Add unit tests for Application.ticker= Add unit tests for Application.ticker setter Mar 2, 2018
@bigtimebuddy bigtimebuddy added Type: Enhancement 💾 v4.x (Legacy) Legacy version 4 support 🥶 Low Priority Generally issues or PRs that don’t need to make it into the next release. labels Mar 2, 2018
@schipiga
Copy link
Contributor Author

schipiga commented Mar 2, 2018

hi @bigtimebuddy, thank you for code review. I will fix it later. Yeah, I would like to join to pixijs slack

@bigtimebuddy
Copy link
Member

Just send me your email address: matt@mattkarl.com

@schipiga schipiga force-pushed the tests/application-set-ticker branch 4 times, most recently from dc8cf70 to 8272b5c Compare March 3, 2018 07:53
@schipiga
Copy link
Contributor Author

schipiga commented Mar 3, 2018

Hi @bigtimebuddy,

Could you please review it again. I seem, I also fix typo, that destroy didn't consider that ticker may not be, but set ticker gives such ability.

@schipiga schipiga force-pushed the tests/application-set-ticker branch 3 times, most recently from d0e119b to 3c76088 Compare March 3, 2018 09:56

describe('set ticker', function ()
{
const self = this;
Copy link
Member

Choose a reason for hiding this comment

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

Looking much better. One small change can you remove self and just use this within your tests. Means the same thing. No need to cache this

Copy link
Contributor Author

@schipiga schipiga Mar 4, 2018

Choose a reason for hiding this comment

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

@bigtimebuddy
in this case I will use it('<test name>', () => instead of it('<test name>', function (), in order to use this inside a test. Is it ok? As I remember, you wrote:

Please remove the use of the fat-arrow => with Mocha, it actually loses the global context, which we'd like to retain for handling variables and other useful bits (like setting timeouts, slow, etc). As a practice, we keep that in using function.

Copy link
Member

Choose a reason for hiding this comment

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

Actually in Mocha it’s all the same context, whether inside it, or containing describe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, cool! didn't know, thank you, fixed it

@schipiga schipiga force-pushed the tests/application-set-ticker branch from 3c76088 to 18b247d Compare March 4, 2018 18:54
@schipiga
Copy link
Contributor Author

schipiga commented Mar 4, 2018

@bigtimebuddy , could you please review again. As I believe, I fixed all

Copy link
Member

@bigtimebuddy bigtimebuddy left a comment

Choose a reason for hiding this comment

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

Nice one, thanks @schipiga.

And good first PixiJS PR!

@schipiga
Copy link
Contributor Author

schipiga commented Mar 4, 2018

thank you :)

@bigtimebuddy bigtimebuddy merged commit 2497379 into pixijs:dev Mar 5, 2018
schipiga added a commit to schipiga/pixi.js that referenced this pull request Mar 9, 2018
@lock
Copy link

lock bot commented Mar 5, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Mar 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🥶 Low Priority Generally issues or PRs that don’t need to make it into the next release. 💾 v4.x (Legacy) Legacy version 4 support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants