[SeaMonkey] xpcshell-tests: test_expire.js fails since bug 390158 landing

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Toolkit
Form Manager
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: sgautherie, Assigned: Ian Neal)

Tracking

({verified1.9.1})

1.9.1 Branch
mozilla1.9.3a1
verified1.9.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(status1.9.2 beta1-fixed, status1.9.1 .4-fixed)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
{
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey2.0/1252756268.1252762151.19494.gz
Linux comm-1.9.1 unit test on 2009/09/12 04:51:08

TEST-UNEXPECTED-FAIL | /builds/slave/comm-1.9.1-linux-unittest/build/objdir/mozilla/_tests/xpcshell/test_satchel/unit/test_expire.js | true == false - See following stack:
JS frame :: /builds/slave/comm-1.9.1-linux-unittest/build/mozilla/testing/xpcshell/head.js :: do_throw :: line 159
JS frame :: /builds/slave/comm-1.9.1-linux-unittest/build/mozilla/testing/xpcshell/head.js :: do_check_eq :: line 189
JS frame :: /builds/slave/comm-1.9.1-linux-unittest/build/mozilla/testing/xpcshell/head.js :: do_check_false :: line 208
JS frame :: /builds/slave/comm-1.9.1-linux-unittest/build/objdir/mozilla/_tests/xpcshell/test_satchel/unit/test_expire.js :: run_test :: line 155
+
TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.clearUserPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: /builds/slave/comm-1.9.1-linux-unittest/build/objdir/mozilla/_tests/xpcshell/test_satchel/unit/test_expire.js :: run_test :: line 194"  data: no]
}

Something is wrong related to "browser.formfill.expire_days"...

Regression timeframe:
http://hg.mozilla.org/comm-central/pushloghtml?fromchange=04542a716ea1&tochange=e010e16d7983
Bug 390158!
Flags: blocking-seamonkey2.0?
(Reporter)

Updated

8 years ago
Summary: [SeaMonkey] xpcshell-tests: test_expire.js fails sicne bug 390158 landing → [SeaMonkey] xpcshell-tests: test_expire.js fails since bug 390158 landing

Comment 1

8 years ago
Is the pref error occuring because the first test failure stops the test run before the pref has been set? That would mean that we can't clear it. I can't work out what's going wrong with that test though, the error's unclear :-(

Comment 2

8 years ago
I think Firefox and probably the test is relying of the fallback of the pref to the history expire pref, which I'm also not too glad that we lost with offering a UI pref for it.

Comment 3

8 years ago
Not blocking release on something that is not clear that it's a real error. Let's find out first if the bug is in our code or the tests and if it's in our code (which I doubt somehow), then nominate again.
Flags: blocking-seamonkey2.0? → blocking-seamonkey2.0-
(Reporter)

Comment 4

8 years ago
(In reply to comment #1)

> Is the pref error occuring because the first test failure stops the test run
> before the pref has been set? That would mean that we can't clear it.

Indeed.  m-1.9.1 needs "try+catch{ clearUserPref() }".

> I can't
> work out what's going wrong with that test though, the error's unclear :-(

{
153   triggerExpiration();
154 
155   do_check_false(fh.entryExists("179DaysOld", "foo"));
}
fails, as if triggerExpiration() did not expire this entry...

Comment 5

8 years ago
yes, very much looks like the usual need for a try/catch on clearUserPref() - on m-c the code was changed so that a clearUserPref() doesn't throw when the set value is the same as the default, but on 1.9.1 is actually does throw (needlessly) in that case.
(Assignee)

Comment 6

8 years ago
This is strange. The only change that could possibly affect this is adding a default value to the pref "browser.formfill.expire_days" and the test passes that part (lines 129-132).
I added a check after line 147 to make sure "browser.formfill.expire_days" was set to 30 and it was, so I cannot see any change from bug 390158 causing this.
(Assignee)

Comment 7

8 years ago
Ah, I see why, line 147 sets "browser.history_expire_days" not "browser.formfill.expire_days" so of course the form history does not get expired.

Comment 8

8 years ago
(In reply to comment #7)
> Ah, I see why, line 147 sets "browser.history_expire_days" not
> "browser.formfill.expire_days" so of course the form history does not get
> expired.

Yes, it seems it also tests that we are falling back to the history pref when the formfill one isn't set. Of course, with SeaMonkey now setting the formfill one, that part of the test fails.
(Assignee)

Comment 9

8 years ago
Created attachment 402669 [details] [diff] [review]
Revise test_expire and formfill.expire_days default patch v0.1

This patch:
* Sets browser.formfill.expire_days to 180 for firefox (which is what it falls back to with browser.history_expire_days)
* Sets formfill as well as history expire_days in test_expire.js so that SM does not fail the test.
Assignee: nobody → iann_bugzilla
Status: NEW → ASSIGNED
Attachment #402669 - Flags: review?(sdwilsh)
(Assignee)

Updated

8 years ago
Component: Preferences → Form Manager
Flags: blocking-seamonkey2.0-
Product: SeaMonkey → Toolkit
QA Contact: preferences → form.manager
(Assignee)

Comment 10

8 years ago
Created attachment 402672 [details] [diff] [review]
Set formfill.expire_days pref rather than falling back to history_expire_days pref patch v0.1 [Checkin: Comment 22]

This patch:
* Sets a default for browser.formfill.expire_days rather than falling back to the browser.history_expire_days preference.
Attachment #402672 - Flags: superreview?(bienvenu)
Attachment #402672 - Flags: review?(bienvenu)
(Assignee)

Comment 11

8 years ago
(In reply to comment #9)
> Created an attachment (id=402669) [details]
> Revise test_expire and formfill.expire_days default patch v0.1
> 
> This patch:
> * Sets browser.formfill.expire_days to 180 for firefox (which is what it falls
> back to with browser.history_expire_days)
> * Sets formfill as well as history expire_days in test_expire.js so that SM
> does not fail the test.

I suppose could also remove lines 552-553 from nsStorageFormHistory.cpp if there is a default for browser.formfill.expire_days

Updated

8 years ago
Attachment #402672 - Flags: superreview?(bienvenu)
Attachment #402672 - Flags: superreview+
Attachment #402672 - Flags: review?(bienvenu)
Attachment #402672 - Flags: review+
(Assignee)

Comment 12

8 years ago
Comment on attachment 402672 [details] [diff] [review]
Set formfill.expire_days pref rather than falling back to history_expire_days pref patch v0.1 [Checkin: Comment 22]

Requesting a=tb3 for this simple, low risk patch
Attachment #402672 - Flags: approval-thunderbird3?
Attachment #402672 - Flags: approval-thunderbird3? → approval-thunderbird3+
(Assignee)

Comment 13

8 years ago
Comment on attachment 402672 [details] [diff] [review]
Set formfill.expire_days pref rather than falling back to history_expire_days pref patch v0.1 [Checkin: Comment 22]

http://hg.mozilla.org/comm-central/rev/9cd6b5097325
Attachment #402672 - Attachment description: Set formfill.expire_days pref rather than falling back to history_expire_days pref patch v0.1 → Set formfill.expire_days pref rather than falling back to history_expire_days pref patch v0.1 [Checkin: Comment 13]
(Assignee)

Updated

8 years ago
Version: Trunk → 1.9.1 Branch
Comment on attachment 402672 [details] [diff] [review]
Set formfill.expire_days pref rather than falling back to history_expire_days pref patch v0.1 [Checkin: Comment 22]

Backed out again, since it isn't clear from the bug that everyone understood that the effect of this patch would be to cause Thunderbird to have the same failure that SeaMonkey has, leaving us orange (at a rather stressful time) until whenever if ever you get to land the other patch on 1.9.1.
Attachment #402672 - Attachment description: Set formfill.expire_days pref rather than falling back to history_expire_days pref patch v0.1 [Checkin: Comment 13] → Set formfill.expire_days pref rather than falling back to history_expire_days pref patch v0.1
Comment on attachment 402669 [details] [diff] [review]
Revise test_expire and formfill.expire_days default patch v0.1

r=sdwilsh
Attachment #402669 - Flags: review?(sdwilsh) → review+
(Assignee)

Updated

8 years ago
Attachment #402669 - Flags: approval1.9.1.4?
Comment on attachment 402669 [details] [diff] [review]
Revise test_expire and formfill.expire_days default patch v0.1

Uhh, no, this isn't right. The intention is for form history to use the same expiration period as places. The browser.formfill.expire_days pref is just supported as a hidden pref. A couple people had commented that they might want to have different expiration periods, and it was easy to add.
Attachment #402669 - Flags: review+ → review-
(Assignee)

Updated

8 years ago
Attachment #402669 - Flags: approval1.9.1.4?
(Assignee)

Comment 17

8 years ago
Created attachment 403330 [details] [diff] [review]
Revise test_expire so works with SeaMonkey patch v0.1a [Checkin: Comment 21]

Changes since v0.1:
* Removed changes to firefox.js
* Made formfill.expire_days pref not get set for firefox.
Attachment #402669 - Attachment is obsolete: true
Attachment #403330 - Flags: review?(dolske)
Comment on attachment 403330 [details] [diff] [review]
Revise test_expire so works with SeaMonkey patch v0.1a [Checkin: Comment 21]

I'd add a longer comment there, noting that this is because SM/TB include a default value for browser.formfill.expire_days, so it has to be changed instead of the other pref here.
Attachment #403330 - Flags: review?(dolske) → review+
(Assignee)

Comment 19

8 years ago
Comment on attachment 403330 [details] [diff] [review]
Revise test_expire so works with SeaMonkey patch v0.1a [Checkin: Comment 21]

I'd like to get this into 1.9.1.x in time for SM2.0 freeze (1st October) so that we're not orange on tests.
Attachment #403330 - Flags: approval1.9.1.4?
Comment on attachment 403330 [details] [diff] [review]
Revise test_expire so works with SeaMonkey patch v0.1a [Checkin: Comment 21]

Test-only changes don't need approval. Land away!
Attachment #403330 - Flags: approval1.9.1.4?
(Assignee)

Comment 21

8 years ago
Comment on attachment 403330 [details] [diff] [review]
Revise test_expire so works with SeaMonkey patch v0.1a [Checkin: Comment 21]

Pushed to 1.9.1.x - http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e0a5781148f3
Pushed to 1.9.2.x - http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d045dda5ceb9
Pushed to mozilla-central - http://hg.mozilla.org/mozilla-central/rev/d6a7aae2bd04
Attachment #403330 - Attachment description: Revise test_expire so works with SeaMonkey patch v0.1a → Revise test_expire so works with SeaMonkey patch v0.1a [Checkin: Comment 21]
(Assignee)

Comment 22

8 years ago
Comment on attachment 402672 [details] [diff] [review]
Set formfill.expire_days pref rather than falling back to history_expire_days pref patch v0.1 [Checkin: Comment 22]

http://hg.mozilla.org/comm-central/rev/8ee1764e1ced
Attachment #402672 - Attachment description: Set formfill.expire_days pref rather than falling back to history_expire_days pref patch v0.1 → Set formfill.expire_days pref rather than falling back to history_expire_days pref patch v0.1 [Checkin: Comment 22]
(Assignee)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: regression → fixed-seamonkey2.0
Resolution: --- → FIXED
(Reporter)

Comment 23

8 years ago
verified1.9.1, per tinderboxes.
status1.9.1: --- → .4-fixed
status1.9.2: --- → beta1-fixed
Keywords: fixed-seamonkey2.0 → verified1.9.1
Target Milestone: --- → mozilla1.9.3a1
You need to log in before you can comment on or make changes to this bug.