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

make alert protractor testable #3982

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@vlajos

vlajos commented Jul 21, 2015

Basicall using the $interval module. More details: angular/protractor#169 (comment)

@vlajos vlajos force-pushed the vlajos:patch-1 branch 2 times, most recently from 212bc6a to fa4d779 Jul 21, 2015

@vlajos

This comment has been minimized.

vlajos commented Jul 21, 2015

(Sorry for the couple of rebases... this $interval is a bit new for me.)

@wesleycho

This comment has been minimized.

Member

wesleycho commented Jul 22, 2015

I'm hesitant on this - this smells like a hack, and one I don't believe that should be supported.

I would try resolving the issue using the other suggested approach in that issue of ptor.ignoreSynchronization

@vlajos

This comment has been minimized.

vlajos commented Jul 22, 2015

I think both solutions are a bit hack like.
But hopefully the $interval based solution makes all the library's users life easier.
They won't see the obscure error message, they won't have to figure out that the underlying $timeout is the problem and they won't have to implement a hack.
I think this is the most important difference.

alertCtrl.close();
$interval.cancel(intervalCanceller);

This comment has been minimized.

@petrjasek

petrjasek Jul 23, 2015

not sure how this works with protractor, but it should be enough to set count to 1:

$interval(function() {
     alertCtrl.close();
}, parseInt(attrs.dismissOnTimeout, 10), 1);

This comment has been minimized.

@vlajos

vlajos Jul 23, 2015

@petrjasek Thanks! You are right. I have just updated this change.

make alert protractor testable
Basicall using the `$interval` module. More details: angular/protractor#169 (comment)

@vlajos vlajos force-pushed the vlajos:patch-1 branch from fa4d779 to 6cefa02 Jul 23, 2015

@wesleycho

This comment has been minimized.

Member

wesleycho commented Aug 8, 2015

Problem is, this is a hack that is a completely incorrect use of $interval for what is very clearly a $timeout situation. This should be fixed in Protractor itself if this is a problem.

Closing as invalid.

@wesleycho wesleycho closed this Aug 8, 2015

@vlajos

This comment has been minimized.

vlajos commented Aug 8, 2015

OK. No problem.
However currently testing this project with protractor is a bit painful.
Could you please maybe contact them and explain your arguments?
It would be good to find an acceptable solution for both projects making the shared users life a bit easier. :-)

@Foxandxss

This comment has been minimized.

Member

Foxandxss commented Aug 11, 2015

Even if I agree with @wesleycho in here, I had to make those changes to my toastr as well. They are not that problematic to merge and won't change the app code that much.

@adiherzog

This comment has been minimized.

adiherzog commented Sep 16, 2015

I would have appreciated if you had merged this small change. Now I had to implement my own closeAlert function so that the $interval service is used. Protractor suggests to use the ignoreSynchronization setting, but I don't think that would be a superior solution. Protractor does not show any signs of fixing it on their side either (angular/protractor#169).

@Foxandxss

This comment has been minimized.

Member

Foxandxss commented Sep 16, 2015

I will probably merge it for the next version.

@Foxandxss Foxandxss reopened this Sep 16, 2015

@pkozlowski-opensource

This comment has been minimized.

Member

pkozlowski-opensource commented Sep 16, 2015

@Foxandxss how this would be a valid fix? $interval will keep firing, after initial close...

I'm worried here that we start to hack around bugs in other places and I agree with @wesleycho that we should fix the bug where it originates.

@Foxandxss

This comment has been minimized.

Member

Foxandxss commented Sep 16, 2015

@pkozlowski-opensource You can make $interval to fire once. Basically make it work as a single $timeout. It won't make any difference at all in how it would work. It won't fire after initial close.

I agree that it is not the proper solution, better to have it fixed on Protractor than hacked on a library. My point is that sometimes we shouldn't simply say: Not my problem, wontfix.

This bug on protractor is well known and it haven't been fixed for a year (or even more). So we can simply ignore that it exists and make the end users angry because they will have to make even a bigger hack to be able to test, or we can merge a really simple hack, that won't do us any harm at all. It is not like we are adding an extra $timeout, that is an horrible hack, we are just swapping $timeout for a single time $injector. That would make the exact same behavior and all the ui-bootstrap users that does protractor testing would be really happy. If the Protractor teams fix it, we can simply revert it back.

I am not a member of the protractor library, so I can only speak for my users, and they need a solution for a bug that for some reason or another, is not fixed yet.

I had to fix this in a library of mine as well Foxandxss/angular-toastr#28 almost a year ago and everything works perfect.


Said that, I agree with both of you, in a good day, we won't need hacks, but the proper solution is out of my reach so I only worry for ui-bootstrap users.

@vlajos

This comment has been minimized.

vlajos commented Sep 16, 2015

(In this given patch $interval fires once.)

@pkozlowski-opensource

This comment has been minimized.

Member

pkozlowski-opensource commented Sep 16, 2015

My point is that sometimes we shouldn't simply say: Not my problem, wontfix.

It is not my point either. I'm just proposing a different action here: instead of hacking on multiple libraries (yours, here and probably hundreds of others) let's fix it at the source?

the proper solution is out of my reach

Why are you saying so? IMO focusing on a fix at the source would be more "economical" as compared to work-around in many libraries facing the same issue.

@Foxandxss

This comment has been minimized.

Member

Foxandxss commented Sep 16, 2015

Out of my reach because Protractor is not a library of my competence.

Anyhow, looking at the protractor issue, seems like Julie recommended to use the $interval hack to solve this issue, you can see it here: angular/protractor#169 (comment)

Then she closed the issue.

If you think that it is still something that should be fixed in Protractor, let's talk with Julie again and close this issue. Best solution is always recommended, but sometimes that is not that clear which one is.

@wesleycho

This comment has been minimized.

Member

wesleycho commented Sep 16, 2015

My preference is that this should be fixed in Protractor - otherwise, what is the point in ever using $timeout? It has a strong smell to me, since this workaround would force a core piece of browser functionality to be avoided in favor of one that can be used in the same way, but not intended to. In addition, the browser probably has setTimeout optimized over setInterval for this use case, and we would lose any optimizations present by making this switch.

@Foxandxss

This comment has been minimized.

Member

Foxandxss commented Sep 16, 2015

That is fine for me then 👍

@rcollette

This comment has been minimized.

rcollette commented Sep 6, 2016

After reading lots of long threads in lots of places about this issue, it turns out $interval was specifically intended to provide a mechanism for creating timers/intervals that wouldn't affect Protractor synchronization. It's not just some random alternate function whose usage doesn't match the intent of long running timers. The "fix" was to add it to angular core and not protractor. The intent is right there in the commit comment.
angular/angular.js@2b5ce84

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment