TPS should validate the client/server bookmark state (with bookmark_validator.js) after each phase.

Review commit: https://reviewboard.mozilla.org/r/54018/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/54018/

Comment on attachment 8754531 [details] MozReview Request: Bug 1274394 - Run the bookmark validator after each phase of TPS tests that touched bookmarks r?markh Is this what you had in mind? TBH I think that probably we should only run it if the bookmarks engine has been touched at all, but maybe it's not going to hammer the server that hard. (Also this has a decent number of things that would be in common with bug 1267917, but I'm not sure if it's worth doing those in this patch rather than refactoring this code to use the changes from 1267917 when it eventually lands). In order to make it work with mozreview this patch applies on top of inbound, and so most tests fail due to not having the changes from bug 1273234. Let me know if I should add a patch that works off the latest push to 1273234... I wasn't sure how this should be handled.

https://reviewboard.mozilla.org/r/54018/#review50778 This looks great, and I agree we should only do the validation after touching the bookmarks if that's not too difficult to arrange. Re sharing with bug 1267917, I'm really not sure, although it does strike me that it would be nice for the validation module to have some kind of ability to summarize - eg, something that returns an array of [problemKey, count] or something, so the callers don't all need to know exactly which elements to enumerate and whether they are simple arrays or counts - and that sounds like it would work for telemetry (but not for, eg, the addon). What do you think?

Comment on attachment 8754531 [details] MozReview Request: Bug 1274394 - Run the bookmark validator after each phase of TPS tests that touched bookmarks r?markh eg, let's say we add a new field to the validator (like serverUnexpected added just yesterday) or even removed them, it would be great if TPS "just worked" and we didn't need to remember to update that code ('cos we will forget at some stage)

https://reviewboard.mozilla.org/r/54018/#review50778 A way of getting a results summary is a good idea. Having to update multiple places when a new field is added would be a chore and easy to mess up on, even if it was listed in a comment or whatever.

Comment on attachment 8754531 [details] MozReview Request: Bug 1274394 - Run the bookmark validator after each phase of TPS tests that touched bookmarks r?markh Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54018/diff/1-2/

Comment on attachment 8754531 [details] MozReview Request: Bug 1274394 - Run the bookmark validator after each phase of TPS tests that touched bookmarks r?markh Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54018/diff/2-3/

Comment on attachment 8754531 [details] MozReview Request: Bug 1274394 - Run the bookmark validator after each phase of TPS tests that touched bookmarks r?markh https://reviewboard.mozilla.org/r/54018/#review52066 Looks great, thanks - it looks like you've checked it doesn't cause failures locally given the comments, but if you haven't please do so before landing. ::: services/sync/tps/extensions/tps/resource/tps.jsm:594 (Diff revision 3) > + > + > + /** > + * Use Sync's bookmark validation code to see if we've corrupted the tree. > + */ > + ValidateBookmarks: function TPS__ValidateBookmarks() { We tend to use "new style" function declarations when adding new stuff even when it is inconsistent with the rest of the code - so just make this ValidateBookmarks() {...}

Comment on attachment 8754531 [details] MozReview Request: Bug 1274394 - Run the bookmark validator after each phase of TPS tests that touched bookmarks r?markh Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54018/diff/3-4/

https://reviewboard.mozilla.org/r/54018/#review52066 > We tend to use "new style" function declarations when adding new stuff even when it is inconsistent with the rest of the code - so just make this ValidateBookmarks() {...} Thanks, I had been worndering about the right thing to do here.

*wondering. Anyway, is it worth integrating this know, knowing that it will cause many new TPS failures?

(In reply to Thom Chiovoloni [:tcsc] from comment #11) > *wondering. > > Anyway, is it worth integrating this know, knowing that it will cause many > new TPS failures? No. I thought your block: > if (name === "serverUnexpected" && problemData.serverUnexpected.indexOf("mobile") >= 0) { > // Exclude this hackily so that we don't report this every time until > // we figure out what to do with it. See bug 1273234 and 1274394 for > // more information. was there to prevent that? IMO we should just add more similar blocks until it passes.

Even in the cases where it appears to be a legitimate issue? For example, I get several issues with parentName in test_sync.js, which (IMO) is likely to be a real bug, and given that IIUC parentName is used during reconciliation, it seems somewhat bad to ignore... That said, going over this again made me realize there was one case where the bookmark client/server state was left intentionally unsynced at the end of a phase, so I added the ability to opt-out of the autovalidate, and had that test do so. Incoming patch ignores parentName issues, and fixes that.

Comment on attachment 8754531 [details] MozReview Request: Bug 1274394 - Run the bookmark validator after each phase of TPS tests that touched bookmarks r?markh Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54018/diff/4-5/

Comment on attachment 8754531 [details] MozReview Request: Bug 1274394 - Run the bookmark validator after each phase of TPS tests that touched bookmarks r?markh https://reviewboard.mozilla.org/r/54018/#review52766 ::: services/sync/tps/extensions/tps/resource/tps.jsm:1061 (Diff revisions 4 - 5) > TPS.HandleBookmarks(bookmarks, ACTION_VERIFY); > }, > verifyNot: function Bookmarks__verifyNot(bookmarks) { > TPS.HandleBookmarks(bookmarks, ACTION_VERIFY_NOT); > + }, > + preventValidate() { I think |skipValidation| is a better name

Comment on attachment 8754531 [details] MozReview Request: Bug 1274394 - Run the bookmark validator after each phase of TPS tests that touched bookmarks r?markh (In reply to Thom Chiovoloni [:tcsc] from comment #13) > Even in the cases where it appears to be a legitimate issue? Yes - we should open a bug on the legitimate issue and reference it there. > For example, I get several issues with parentName in test_sync.js, which > (IMO) is likely to be a real bug, and given that IIUC parentName is used > during reconciliation, it seems somewhat bad to ignore... Yes, we can't ignore it forever, but the alternative seems worse IMO. Until we start getting green TPS runs no one is ever going to look at the results - which means we will also never see future regressions. IOW, we have 2 choices here: 1) Make green TPS runs even further away, meaning everyone will assume the failures are the "same old" failures, meaning no one is ever looking at them. 2) Have green runs that exclude "known" problems, meaning people *will* start looking at red runs, so obscure issues or future regressions are noticed. There seems far more value in (2). I just assigned bug 1275125 to you for this reason - TPS isn't adding any value, validator or otherwise, while it's permanently red. > That said, going over this again made me realize there was one case where > the bookmark client/server state was left intentionally unsynced at the end > of a phase, so I added the ability to opt-out of the autovalidate, and had > that test do so. Great. > Incoming patch ignores parentName issues, and fixes that. We'll want a bug on file for that and a reference to it in the comment. When filing that bug, please try and dig into exactly which TPS tests are affected (I doubt it is all of them) to help diagnosis of that new bug. FWIW though, I'm pretty sure that a folder rename does not cause record updates to all children, so it sounds like this is similar to the reason "pos" is often wrong even without a mobile device - ie, the first bullet points in bug 1228827 comment 5 probably apply to parentName too. (There might also be scope for using the same facility used to skip validation here - ie, so individual tests can skip individual bits of the validation? I'm not too bothered by that though if there is exactly 1 such known issue today) I'm also still not clear on whether the tests actually pass with this patch or whether it will cause some tests to fail?

Comment on attachment 8754531 [details] MozReview Request: Bug 1274394 - Run the bookmark validator after each phase of TPS tests that touched bookmarks r?markh Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54018/diff/5-6/

(In reply to Mark Hammond [:markh] from comment #16) > There seems far more value in (2). I just assigned bug 1275125 to you for > this reason - TPS isn't adding any value, validator or otherwise, while it's > permanently red. Makes sense to me. > We'll want a bug on file for that and a reference to it in the comment. When > filing that bug, please try and dig into exactly which TPS tests are > affected (I doubt it is all of them) to help diagnosis of that new bug. FWIW > though, I'm pretty sure that a folder rename does not cause record updates > to all children, so it sounds like this is similar to the reason "pos" is > often wrong even without a mobile device - ie, the first bullet points in > bug 1228827 comment 5 probably apply to parentName too. Done (bug 1276969). It's just test_sync.js, it's the only test that renames a folder AFAICT. > (There might also be scope for using the same facility used to skip > validation here - ie, so individual tests can skip individual bits of the > validation? I'm not too bothered by that though if there is exactly 1 such > known issue today) If you mean just using Bookmarks.skipValidation in the test, IMO this isn't as good of an idea, since testing the renaming of folders isn't the only thing that that test does (it seems to be a fairly broad test). I also think it would need to be used at the end of every phase after the one that renames the folder. If you mean there should be arguments to skipValidation about what kinds of things to ignore validation errors for, maybe eventually, but at the moment this is the only thing. > I'm also still not clear on whether the tests actually pass with this patch > or whether it will cause some tests to fail? Everything should pass after bug 1276152 (the `pos` fix) lands.

Comment on attachment 8754531 [details] MozReview Request: Bug 1274394 - Run the bookmark validator after each phase of TPS tests that touched bookmarks r?markh Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54018/diff/6-7/

Comment on attachment 8754531 [details] MozReview Request: Bug 1274394 - Run the bookmark validator after each phase of TPS tests that touched bookmarks r?markh https://reviewboard.mozilla.org/r/54018/#review53442 Awesome, thanks. > If you mean there should be arguments to skipValidation about what kinds of > things to ignore validation errors for, maybe eventually, but at the moment > this is the only thing. Yeah, that's what I meant, and I agree it's not something to worry about now (and hopefully never ;)

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/d721c08fadf0 Run the bookmark validator after each phase of TPS tests that touched bookmarks. r=markh