Closed Bug 984572 Opened 7 years ago Closed 7 years ago
[Calendar] JSHint fixes for js/*, js/utils and js/worker
fix JSHint errors for files at the top-level "js" folder, "js/utils" and "js/worker"
Comment on attachment 8399038 [details] [review] Pull request Hi Miller, Please help me to review the patch. Thanks.
Attachment #8399038 - Flags: review?(mmedeiros)
Comment on attachment 8399038 [details] [review] Pull request Travis is green, so let's do it. Probably better to have the globals specified per file so we can understand our dependencies a bit better, but this looks good to me.
Attachment #8399038 - Flags: review?(mmedeiros) → review+
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Hi Kevin, Thanks for your help. :)
Darn, I could be wrong but I am fairly sure what's causing this is this line: https://github.com/mozilla-b2g/gaia/pull/17779/files#diff-6bbe7f35ee63a1bab8c2df5cf6907d5fR21 I know we have caldav tests, so I'm wondering how our CI suite didn't catch this. Evan - can you also investigate if we can add a test that would catch this?
There were a few conflicts backing this out, so I went ahead and took care of it: https://github.com/mozilla-b2g/gaia/commit/92d4447a9ee72a49a09410143c9cb3068f73cd53
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Hi Kevin, I'm investigating that.
Hi Kevin, The root cause of Bug 991896 is that we removed https://github.com/evanxd/gaia/blob/e05b0063fbb6139c2df8910e2b09590fad6b77d9/apps/calendar/js/worker/thread.js#L8-L11 before. After we added these lines back, it could work now. But we might not write a uni test for that, because we could not let `window` as undefined in our unit test runner. Or we could write a marionette test for it? But the test could not run on the try server(tbpl). How do you think? And could you help me to review the patch? Thanks.
Comment on attachment 8402514 [details] [review] Pull request Nice find. I would love to prioritize adding a marionette test for this functionality if possible in the future. I tested the add account flow and everything looks good, so R+ from me. https://github.com/mozilla-b2g/gaia/commit/07eabe06e407be0411466aeeb6efdb99bb648aaa
Attachment #8402514 - Flags: review?(kgrandon) → review+
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
(In reply to Kevin Grandon :kgrandon from comment #11) > Comment on attachment 8402514 [details] [review] > Pull request > > Nice find. I would love to prioritize adding a marionette test for this > functionality if possible in the future. Sure, let's do this in Bug 994017.
You need to log in before you can comment on or make changes to this bug.