Closed Bug 984572 Opened 7 years ago Closed 7 years ago

[Calendar] JSHint fixes for js/*, js/utils and js/worker

Categories

(Firefox OS Graveyard :: Gaia::Calendar, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S5 (11apr)

People

(Reporter: mmedeiros, Assigned: evanxd)

References

Details

(Whiteboard: [p=1])

Attachments

(1 file, 1 obsolete file)

46 bytes, text/x-github-pull-request
kgrandon
: review+
Details | Review
fix JSHint errors for files at the top-level "js" folder, "js/utils" and "js/worker"
Assignee: nobody → evanxd
Target Milestone: --- → 1.4 S4 (28mar)
Attached file Pull request (obsolete) —
Comment on attachment 8399038 [details] [review]
Pull request

Hi Miller,

Please help me to review the patch.
Thanks.
Attachment #8399038 - Flags: review?(mmedeiros)
Status: NEW → ASSIGNED
Whiteboard: [priority][p=1]
Whiteboard: [priority][p=1] → [p=1]
Target Milestone: 1.4 S4 (28mar) → 1.4 S5 (11apr)
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+
Landed: https://github.com/mozilla-b2g/gaia/commit/5ed8fe436345eaf5c9f4f6d8e8cd1170da10ca9d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Hi Kevin,

Thanks for your help. :)
Depends on: 991896
Flagging for checkin-needed for a backout for causing bug 991896.
Keywords: checkin-needed
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?
Flags: needinfo?(evanxd)
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
Keywords: checkin-needed
Resolution: FIXED → ---
Hi Kevin,

I'm investigating that.
Flags: needinfo?(evanxd)
Attached file Pull request
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.
Attachment #8399038 - Attachment is obsolete: true
Attachment #8402514 - Flags: review?(kgrandon)
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 ago7 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.