Closed Bug 811490 Opened 7 years ago Closed 7 years ago

Convert services/sync/tests/tps/test_privbrw_tabs.js to PB per window mode

Categories

(Testing Graveyard :: TPS, defect)

defect
Not set

Tracking

(firefox20 fixed)

RESOLVED FIXED
mozilla20
Tracking Status
firefox20 --- fixed

People

(Reporter: andreshm, Assigned: andreshm)

References

(Blocks 1 open bug)

Details

(Whiteboard: [appcoast])

Attachments

(2 files)

Tabs engine have been updated to use PB per window. 

We need to convert services/sync/tests/tps/test_privbrw_tabs.js to handle PB per window.  

Follow up bug from Bug 722977 Comment 19.
Attached patch Patch v1Splinter Review
Attachment #682262 - Flags: review?(ehsan)
Attachment #682262 - Flags: review?(jgriffin)
Blocks: pbngentest
Comment on attachment 682262 [details] [diff] [review]
Patch v1

Review of attachment 682262 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, although I'm not very familiar with the sync test code.
Attachment #682262 - Flags: review?(rnewman)
Attachment #682262 - Flags: review?(jgriffin)
Attachment #682262 - Flags: review?(ehsan)
Attachment #682262 - Flags: review+
Status: NEW → ASSIGNED
Comment on attachment 682262 [details] [diff] [review]
Patch v1

Review of attachment 682262 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me (there are some things I'd like to fix in tps.jsm, but I'm not going to r- code for following existing conventions!).

I'll file a follow-up to simplify and clean up some of TPS.

::: services/sync/tps/extensions/tps/modules/windows.jsm
@@ +5,5 @@
> + /* This is a JavaScript module (JSM) to be imported via
> +   Components.utils.import() and acts as a singleton.
> +   Only the following listed symbols will exposed on import, and only when
> +   and where imported. */
> +

Use strict in new code, please.
Attachment #682262 - Flags: review?(rnewman) → review+
Component: Firefox Sync: Backend → TPS
Product: Mozilla Services → Testing
Blocks: 812532
Andres, can you please investigate the test failure?  Thanks!
The issue was not related to this patch. Was fixed in the latest patch on Bug 722977 Comment 24.
https://hg.mozilla.org/mozilla-central/rev/9d2464e31941
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
[phase3] Exception caught: Cannot modify properties of a WrappedNative Stack trace: TPS__SetPrivateBrowsing()@resource://tps/tps.jsm:688 < TPS.RunNextTestAction()@resource://tps/tps.jsm:481 < TPS__FinishAsyncOperation/<()@resource://tps/tps.jsm:154 <
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Follow-up. v1Splinter Review
Attachment #700014 - Flags: review?(gps)
Please make sure to uplift this patch to Aurora as well.  Thanks!
(In reply to :Ehsan Akhgari from comment #12)
> Please make sure to uplift this patch to Aurora as well.  Thanks!

TPS doesn't run on Aurora, but sure :)
(In reply to comment #13)
> (In reply to :Ehsan Akhgari from comment #12)
> > Please make sure to uplift this patch to Aurora as well.  Thanks!
> 
> TPS doesn't run on Aurora, but sure :)

Yeah, I'd just like to minimize the difference in the per-window PB related code on central and Aurora...
TPS passes now. Gonna land this on s-c without waiting for Greg's review.
https://hg.mozilla.org/services/services-central/rev/418277a17cb2

When this goes green, I'll merge and flag for approval.
Whiteboard: [appcoast] → [appcoast][fixed in services]
TPS YEAAAAAHHHHH on s-c.

Landed on inbound, DONTBUILD:

https://hg.mozilla.org/integration/mozilla-inbound/rev/776cf96d305f
Comment on attachment 700014 [details] [diff] [review]
Follow-up. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Bug 722977 -- introducing per-window private browsing. Didn't catch all of the TPS Sync tests for private browsing.

User impact if declined: 
  None.

Testing completed (on m-c, etc.): 
  TPS green on s-c.

Risk to taking this patch (and alternatives if risky): 
  None. It's just to keep code aligned between Aurora and trunk; the patch that's now in Aurora is incomplete.

String or UUID changes made by this patch: 
  None.
Attachment #700014 - Flags: review?(gps)
Attachment #700014 - Flags: review+
Attachment #700014 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/776cf96d305f
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Whiteboard: [appcoast][fixed in services] → [appcoast]
Comment on attachment 700014 [details] [diff] [review]
Follow-up. v1

https://bugzilla.mozilla.org/show_bug.cgi?id=804745#c37
Attachment #700014 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.