Permalink
8 comments
on commit
sign in to comment.
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Browse files
Run focus test only if document has focus
(cherry picked from commit 0b9a182)
- Loading branch information
Showing
1 changed file
with
21 additions
and
21 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
33c80f3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't reproduce this on browserstack (version before this change). I'd recommend reverting this change.
33c80f3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Krinkle seems to me this change also affects local testing. I'd rather not have focus tests fail if the document does not have focus.
33c80f3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It requires more thought anyway, since it minimizes the possible failure with focus tests but not removed it, i.e. even with this fix, we have same failure - http://swarm.jquery.org/result/883577.
33c80f3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timmywil That is not the justification for this commit. And I'd argue that is a genuine failure resulting from incorrectly running the tests (I'd rather be annoyed by a failing test because I didn't focus my local browser when testing, then having it say Passed and find out we missed a bug or regression).
@markelog So it is confirmed then that this commit should be reverted as it doesn't help, the premise has been invalidated.
33c80f3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it might be. Besides, we will found out about possible regression in the swarm, if, of course, browserstack browsers always be in focus, otherwise build will always fail, or worse - it will sometimes fail. @dmethvin?
You know story about the kid and wolf? Well, right know, it's like that.
Could you extend on that? Where specifically is problem resides?
I did not said "reverted", i said "requires more thought".
33c80f3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Krinkle I agree with the principle, but we are making an exception in this case. This is one test out of many. If the regression is only in this test, I'd question the test more than the code. Besides, we've had lots of issues in the past with automated focus tests. At this point, we would rather have it pass when the document does not have focus.
33c80f3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A flakey test that leads us to ignore unit tests failing is not good. Focus tests are notoriously unreliable since they depend on factors outside the control of the unit tests. So I support this patch.
33c80f3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmethvin @timmywil @markelog Yes, I could agree (I personally don't, but I can understand).
However in this case I think this commit should be reverted still as it isn't working. The test is still failing in IE8 after this commit as much as it did before this commit: http://swarm.jquery.org/project/jquery. It appears to either be a faulty condition (IE8 is not letting us detect the focus), or the bug is unrelated to the document having focus.