ECOM-3177 fix flaky js tests on dashboard.TrackEvents #11416
Conversation
|
@jzoldak do you have time to review this? |
| window.edx.dashboard.generateProgramProperties] | ||
| ]; | ||
|
|
||
| for (var cnt=0; cnt< expectedArray.length; cnt ++){ |
There was a problem hiding this comment.
nit: index instead of cnt
|
@schenedx , first pass complete |
|
👍 , thanks! |
|
Hmmm tests weren't kicked off for this PR. jenkins ok to test |
|
jenkins run all |
|
@schenedx is there a reason you're not in the edx github org? could you put in a request to #devops to make that happen? once you do that (and make sure your profile is public), your PRs will automatically kick off for you in this repo. |
| findCourseButtonArgs, | ||
| xseriersActionButtonArgs]; | ||
|
|
||
| for (var index=0; index< expectedArgsForTrackLinkCall.length; index ++){ |
There was a problem hiding this comment.
i'm wondering if this line will result in a jshint violation. if it does, please fix it.
There was a problem hiding this comment.
@benpatterson I was able to fix the jshint errors. Thanks for pointing out.
|
I don't fully understand the fix here; I don't see the Or, perhaps I'm overlooking something....Thoughts? |
|
@benpatterson: What I am doing in my fix is to
|
|
jenkins run all |
1 similar comment
|
jenkins run all |
|
jenkins ok to test |
|
@schenedx Sorry if I'm being dense here, but based on your description, I would think that the spy was started/stopped at the wrong time (started before all the tests, stopped after all the tests, as opposed to started/stopped one at a time). I'm still concerned that, in the event of one of the tests failing, it would not be obvious which one failed, because the only assertion that will fail is that the target was called 6 times instead of 7.... |
|
@benpatterson
If one of the "window.analytics.trackLink" calls is missing, here is the error from the unit test:
Base on the message above, and if you look at the code at https://github.com/edx/edx-platform/blob/master/lms/static/js/dashboard/track_events.js , it's clear to me what the problem might be. I feel like the trouble you are having is how the code is written, not how the unit test is written. Do you have a better way to write the unit tests based on the code in track_events.js? |
|
hi @schenedx thanks for taking a closer look. In part, I am concerned about the output (according to the 2nd example you posted, a user must grok more logs to figure out which one failed). But, more generally, the js tests get collected in an xunit-style output file, which clearly calls out which testcase fails. So, a developer that might be looking over a failed test suite, will very easily understand which test failed. For example, in today's world, if the course image link no longer sends events, we'd get a test failure like this: I do agree that your change may help the actual flaky condition, but IMO we are taking things in the wrong direction in terms of making tests easy to understand for someone that might be new to the code. This would be accomplished through discrete tests, as opposed to groups. So, I'm not talking about the underlying production code; I'm talking about the way the tests are composed. Now, again I'm curious about the spying mechanism here, because perhaps that's where you and I are missing each other's points. Can a spy for |
|
The spy is a type of mock. We are basically substituting the "window.analytics.trackLink" function with a spy function within "edx.dashboard.trackEvents". If the function we are testing calls the "trackLink" 7 times, the spy will be used 7 times. I don't see a way to unit test the "edx.dashboard.trackEvents" statement by statement. As far as I know, jasmine unit tests are meant to test the whole function, not individual logic statements within the function. |
|
Ok now I've had a chance to take a closer look. I see how the 2nd expect call can make it more clear which link was not defined with an event emission tied to it. So, thank you for explaining. However, I'm still not sold on grouping them together. If these are flaky, then why do they all fail together? (Because, the spy is set up on each spec, not on the entire file...so I'd expect if there was flakiness in setup, etc, that only on testcase would fail; however, all of them have failed in the past when the tests fail.) So perhaps I don't understand how this was flaky to begin with. Were you able to reproduce the flaky condition? In an earlier comment you'd mentioned toHaveBeenCalledWith is causing the flakiness. Why? I know I have a lot of questions on this code review. Thank you for your patience in addressing them. |
|
@benpatterson If you read the message above carefully, you will see the "toHaveBeenCalledWith" call is complaining the test expected just 1 set of arguments, but the "trackLink" function was called with 7 different sets of arguments. This make sense because the "trackLink" function was called 7 times within "edx.dashboard.trackEvents". |
| $('.xseries-action .btn'), | ||
| 'edx.bi.dashboard.xseries_cta_message.clicked', | ||
| window.edx.dashboard.generateProgramProperties | ||
| ); |
There was a problem hiding this comment.
My reading of toHaveBeenCalledWith seems to say that this call should be fine, since it's just checking that those arguments were called on the spy at least once.
http://jasmine.github.io/2.0/introduction.html#section-Spies
So, I think there may be a different root cause, here.
There was a problem hiding this comment.
@cpennington Thank you for the closer look.
First, I believe we are using Jasmine 1.3 instead of Jasmine 2. Therefore the documentation should be http://jasmine.github.io/1.3/introduction.html#section-Spies
Nevertheless, the 1.3 documentation says the same that "The toHaveBeenCalledWith matcher will return true if the argument list matches any of the recorded calls to the spy."
This explains why I cannot reproduce the issue, and why the original test fails only a couple of times for the past few month.
However, if you eye ball the last error message I collected in the above comments, there should not be any reason for the test to fail.
At this point, I am also questioning what the root cause actually is. Do you think there will be enough data to figure out the root cause?
There was a problem hiding this comment.
If I had to guess, I'd guess that the HTMLNode objects weren't evaluating equal to the spy, perhaps because of a timing problem.
This is the expected call
[
{
0 : HTMLNode,
length : 1,
prevObject : { 0 : HTMLNode, context : HTMLNode, length : 1 },
context : HTMLNode,
selector : '.course-title > a'
},
'edx.bi.dashboard.course_title.clicked',
Function
]
one of the elements in the list of actual calls is
[
{
0 : HTMLNode,
length : 1,
prevObject: { 0 : HTMLNode, context : HTMLNode, length : 1 },
context : HTMLNode,
selector : '.course-title > a'
},
'edx.bi.dashboard.course_title.clicked',
Function
]
The only places that could be different are the various HTMLNodes and the Function calls (neither of which have any identifying info in the print statement).
schenedx
force-pushed
the
schen/ECOM-3177
branch
from
9d0d539
to
1543340
Compare
|
@benpatterson @adampalay @AlasdairSwan @cpennington |
|
@schenedx Can you briefly explain why changing the test this way will fix the flakiness? I'm not following enough of the fix to understand what was varying when the test broke. |
| 'edx.bi.dashboard.verified_info_link.clicked', | ||
| window.edx.dashboard.generateTrackProperties | ||
| ); | ||
| }); | ||
|
|
||
| it('sends an analytics event when the user clicks find courses button', function() { | ||
| var $findCourse = $('.btn-find-courses'); | ||
| var property = { |
There was a problem hiding this comment.
There should only be 1 var per function, comma separating multiple variables.
|
@cpennington |
schenedx
force-pushed
the
schen/ECOM-3177
branch
from
1543340
to
6679915
Compare
|
Once tests pass 👍 |
|
I'd like to hear what @cpennington thinks, but I like this solution better insofar as keeping the specs as separate ones. Thanks for giving this a closer look. |
|
👍, although I wonder if this is likely to bite us again in another test in the future, and whether https://github.com/velesin/jasmine-jquery might help mitigate that across the whole test suite. |
|
@cpennington while I can't talk to whether jasmine-jquery would have helped us here I have found it very useful in other projects. +1 for adding it to edx-platform. |
|
wait, we have jasmine-jquery: https://github.com/edx/edx-platform/blob/master/cms/static/js_test.yml#L51 |
|
Excellent |
|
@benpatterson do you have further comments or questions? If not, I'll merge |
|
Yeah, I thought we might have it. I guess I just wonder if there's something we could be using with it to be more resilient to equality comparison of jquery objects being flaky. |
|
@schenedx you should fix your quality failure. |
schenedx
force-pushed
the
schen/ECOM-3177
branch
from
6679915
to
5716f03
Compare
|
@benpatterson Right. Rookie mistake. Now fixed. |
|
@schenedx thanks. I agree with @cpennington that I'm not 100% convinced this fixes the flakiness; however, it's certainly worth putting into circulation. We can revisit this if the flakiness reappears. Also, I realize you just fixed things up, but your commit message should be more indicative of what you're commiting. I'm seeing "this is a combination of 2 commits." Could it instead be something that would be easy to understand for someone that's reading the logs? |
schenedx
force-pushed
the
schen/ECOM-3177
branch
from
5716f03
to
f6fc711
Compare
|
@benpatterson Got it. I have updated the commit message to something easier to read. |
|
Thanks @schenedx This is a 👍 from me once all tests pass. |
ECOM-3177 fix flaky js tests on dashboard.TrackEvents
@adampalay @benpatterson Please review
FYI @rock345
This is a pure js unit test change.
It's implemented according to the suggestion provided by @adampalay on the jira ticket ECOM-3177
I am not able to really answer why these test cases don't fail every single time before this change, but my hope is after this change, the test would pass all the time unless it catches a real bug.