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

Fix paper-progress-circular infinite loop in acceptance tests #811

Conversation

tyleryasaka
Copy link
Contributor

I recently upgraded ember paper for my project and found that acceptance tests were failing left and right. I finally isolated the issue to this component, and found that renderCircle was getting caught in a recursive loop once the component started being destroyed. The tests were waiting on this component to finish rendering, and they would wait until the tests timed out.

At first glance it looks like I'm doing a lot here, but I'm just wrapping the contents of renderCircle inside an if statement (in fact, the same check that's being performed inside the renderFrame function defined inside this function).

This prevents a scenario where an infinite loop can be created.
@xomaczar
Copy link
Contributor

Got the same issue - we need to merge this in - can you just return if component is destroyed/destroying, will make this PR much easier to review

@xomaczar
Copy link
Contributor

Ping @tyleryasaka

@arschmitz
Copy link

Ran into this also

@xomaczar 100% agree early return is much better but just for future times with similar PR's that cant be so easily made better you can always add ?w=1to url to ignore whitespace changes makes it much nicer to review

https://github.com/miguelcobain/ember-paper/pull/811/files?w=1
gets it down to 2 lines lol

@miguelcobain miguelcobain merged commit 9f04002 into miguelcobain:master Oct 11, 2017
@miguelcobain
Copy link
Owner

This fix was reworked a bit and is now on master. Thanks everyone.

@tyleryasaka tyleryasaka deleted the progress-circular-fix-infinite-loop branch October 11, 2017 14:54
@xomaczar
Copy link
Contributor

@arschmitz it's not about PR readability(it does help), it's more about code organization, maintenance, etc - I try to never wrap many code lines in if block - if I can help it

@arschmitz
Copy link

@xomaczar i know thats why i said

100% agree early return is much better but just for future times with similar PR's that cant be so easily made better

some pr's have large white spaces changes that just cant be avoided

@xomaczar
Copy link
Contributor

👍

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.

None yet

4 participants