Last Comment Bug 649842 - Intermittent failure in browser/components/sessionstore/test/browser/browser_590268.js | tab 4 has correct uniq2 value - Got 1302731209349.896, expected 1302731209351.6611
: Intermittent failure in browser/components/sessionstore/test/browser/browser_...
Status: RESOLVED FIXED
: intermittent-failure
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 6
Assigned To: :Ehsan Akhgari
:
: Mike de Boer [:mikedeboer]
Mentors:
Depends on:
Blocks: 438871 603536 624384
  Show dependency treegraph
 
Reported: 2011-04-13 16:05 PDT by :Ehsan Akhgari
Modified: 2012-11-25 19:31 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (956 bytes, patch)
2011-04-13 16:24 PDT, :Ehsan Akhgari
paul: review+
Details | Diff | Splinter Review

Description :Ehsan Akhgari 2011-04-13 16:05:59 PDT
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1302729049.1302732694.16179.gz
Rev3 MacOSX Leopard 10.5.8 mozilla-central debug test mochitest-other on 2011/04/13 14:10:49

The problem in this test is that it assumes that the values returned by Math.random() will always be unique.  This assumption is false.
Comment 1 Bill Gianopoulos [:WG9s] 2011-04-13 16:11:48 PDT
I am always amused by people who don't understand random numbers.

I have had people tell me that the way to produce unique identifiers is just to generate significantly wrong random numbers.  Random numbers and unique identifiers are really almost polar opposites actually.
Comment 2 :Ehsan Akhgari 2011-04-13 16:24:13 PDT
Created attachment 525852 [details] [diff] [review]
Patch (v1)
Comment 3 :Ehsan Akhgari 2011-04-13 16:49:07 PDT
Setting a dependency on other bugs on tests which use the r() function, they could all be affected by this bug.

Also, I updated the documentation to include this pattern: <https://developer.mozilla.org/en/QA/Avoiding_intermittent_oranges#Tests_that_rely_on_Math.random%28%29.c2.a0to_create_unique_values>
Comment 4 :Ehsan Akhgari 2011-04-13 16:51:00 PDT
(In reply to comment #1)
> I am always amused by people who don't understand random numbers.

It's not a question of understanding the concept.  When you're focusing on testing something, it is extremely common for you to miss something obvious.  I don't think it's good for us to blame anybody for not understanding the difference between unique and random numbers.

Please let's focus on fixing bugs instead of blaming people of not understanding stuff.  Thanks!  :-)
Comment 5 Bill Gianopoulos [:WG9s] 2011-04-13 16:53:49 PDT
(In reply to comment #4)
> (In reply to comment #1)
> > I am always amused by people who don't understand random numbers.
> 
> It's not a question of understanding the concept.  When you're focusing on
> testing something, it is extremely common for you to miss something obvious.  I
> don't think it's good for us to blame anybody for not understanding the
> difference between unique and random numbers.
> 
> Please let's focus on fixing bugs instead of blaming people of not
> understanding stuff.  Thanks!  :-)
Point taken.
Comment 6 Marco Bonardo [::mak] 2011-04-13 17:40:57 PDT
I think you should add to the wiki page the common pitfalls when using Date (or Date.now()). Like the fact it's not guarantee that 2 subsequent Date calls will return different times, and the fact there can be a skew between Date and PR_Now(), so much that one could surprisingly be in the past of the one that was invoked later.
In this case most likely the first part of the random number doesn't change due to timers resolution, so we barely rely on Math.rand().

Rather than fixing this r() function you could provide a good monotonic generator in the testing harness, like do_get_monotonic_rand() and getMonotonicRand(). Or file a follow-up. Sounds better than relying on tests implementing it by themselves.
Comment 7 Marco Bonardo [::mak] 2011-04-13 17:46:41 PDT
Or go for getMonotonicNumber, so we leave the "Rand" word out of the door, for less possible confusion.
Comment 8 Marco Bonardo [::mak] 2011-04-13 17:49:22 PDT
Even if, actually we need a good (and maybe thread-safe?) monotonic generator in the platform itself, for various bugs around using Date.now() for the same purpose.
Sorry for spam :)
Comment 9 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-04-13 23:28:55 PDT
Comment on attachment 525852 [details] [diff] [review]
Patch (v1)

Ah silly me. Thanks for find that.
Comment 10 Treeherder Robot 2011-04-14 06:34:59 PDT
ehsan%mozilla.com
http://tinderbox.mozilla.org/showlog.cgi?log=Cedar/1302752583.1302757384.28150.gz
Rev3 WINNT 6.1 cedar debug test mochitest-other on 2011/04/13 20:43:03

s: talos-r3-w7-006
PROCESS-CRASH | Main app process exited normally | application crashed (minidump found)
Thread 0 (crashed)
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser/browser_590268.js | tab 11 has correct uniq2 value - Got 1302755192327.0117, expected 1302755192331.5708
Comment 12 :Ehsan Akhgari 2011-04-14 11:07:45 PDT
Marco, I thought a bit about adding test framework APIs to generate monotonic numbers.  But I'm still not convinced that it's useful.

The thing is that writing such a function is extremely easy and painless.  The tricky part is figuring out if you need to use such a function.  And once you realize that, the path to writing your own version of getMonotonicNumber is very simple.

Why do you think having that function in the test frameworks would be useful?
Comment 13 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-04-14 13:35:49 PDT
Ehsan: It's not that trivial. While you could use a simple monotonically increasing counter, that doesn't work if the same test is run multiple times.

In the test where I needed this (the CORS tests) I seeded the counter using the Date() object and then monotonically increased from there. However it took a while to figure out which function on Date() to use.

Additionally, having the function in the docs hopefully increases the chances that people keep this in their head. I know I end up reading the mochitest docs on a fairly regular basis looking for help with what lives in SimpleTest.
Comment 14 :Ehsan Akhgari 2011-04-14 20:51:35 PDT
(In reply to comment #13)
> Ehsan: It's not that trivial. While you could use a simple monotonically
> increasing counter, that doesn't work if the same test is run multiple times.
> 
> In the test where I needed this (the CORS tests) I seeded the counter using the
> Date() object and then monotonically increased from there. However it took a
> while to figure out which function on Date() to use.

Can you please point me to the code?  Maybe I can just steal your code? :D

> Additionally, having the function in the docs hopefully increases the chances
> that people keep this in their head. I know I end up reading the mochitest docs
> on a fairly regular basis looking for help with what lives in SimpleTest.

OK, that's a fair point.  Sold!
Comment 15 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-04-15 14:07:43 PDT
Turns out my code was exactly as the one you have. I.e. |Date.now() + counter|

It's somewhat tricky if we want the counts to be unique across tests. Could perhaps SimpleTest.getUnique (or whatever we call it) go to the parent test-harness frame if one exists and query the value from there. And if a outer frame doesn't exist then assume that someone is running the test manually and not in such a rapid order that simply using |Date.now() + counter| will generate unique enough values.

Hmm.. that was a complicated description, I hope it made sense.
Comment 16 Marco Bonardo [::mak] 2011-04-15 14:21:56 PDT
fwiw, Date.now() looks like an unnecessary complication, doesn't add any value to the counter by itself. it's just a bigger number.
Comment 17 :Ehsan Akhgari 2011-04-15 15:35:20 PDT
(In reply to comment #15)
> Turns out my code was exactly as the one you have. I.e. |Date.now() + counter|

Heh, OK.

> It's somewhat tricky if we want the counts to be unique across tests. Could
> perhaps SimpleTest.getUnique (or whatever we call it) go to the parent
> test-harness frame if one exists and query the value from there. And if a outer
> frame doesn't exist then assume that someone is running the test manually and
> not in such a rapid order that simply using |Date.now() + counter| will
> generate unique enough values.

Why would we want the value to be unique across tests?  I mean, in theory, tests are supposed to be independent of one another, right?
Comment 18 :Ehsan Akhgari 2011-04-15 15:35:45 PDT
(In reply to comment #16)
> fwiw, Date.now() looks like an unnecessary complication, doesn't add any value
> to the counter by itself. it's just a bigger number.

I agree, the Date.now() part is a red herring.  It's basically a counter that we're talking about.
Comment 19 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-04-15 19:40:14 PDT
The Date.now() thing is useful if you care about generating unique IDs across multiple runs of the test.

For example in the cross-site XHR tests certain things ended up in caches that persist across test runs. By using IDs that are unique across test runs I could ensure that cache data from previous runs didn't interfere with the new run.
Comment 20 Mounir Lamouri (:mounir) 2011-04-16 01:54:24 PDT
Can't you clean-up the caches with:
Components.classes["@mozilla.org/network/cache-service;1"].
           getService(Components.interfaces.nsICacheService).
           evictEntries(Components.interfaces.nsICache.STORE_ANYWHERE);
Comment 21 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-04-16 02:16:16 PDT
Only the http cache. The CORS implementation has a separate cache that holds something else (cached security info).
Comment 22 :Ehsan Akhgari 2011-04-18 16:38:11 PDT
(In reply to comment #19)
> The Date.now() thing is useful if you care about generating unique IDs across
> multiple runs of the test.

Date.now() doesn't provide uniqueness, really.  ;-)  It's quite possible for a test to call it several times and get the same value back each time, if things are moving fast enough.
Comment 23 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-04-18 16:50:13 PDT
Right, hence the counter as well.

The thing I was trying to solve with the Date.now() thing is to make it possible to load a single test and re-run it multiple times. If you have other ideas for how to do that then I'm all ears.
Comment 24 :Ehsan Akhgari 2011-04-18 19:40:31 PDT
(In reply to comment #23)
> Right, hence the counter as well.
> 
> The thing I was trying to solve with the Date.now() thing is to make it
> possible to load a single test and re-run it multiple times. If you have other
> ideas for how to do that then I'm all ears.

Oh, I thought you're proposing Date.now() instead of the counter, but yeah, this makes sense.

Now, whoever wants to file a bug and assign it to me will get a patch from me.  Let the race begin!
Comment 25 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-04-18 19:50:21 PDT
Done: bug 651015
Comment 26 :Ehsan Akhgari 2011-04-18 20:59:24 PDT
(In reply to comment #25)
> Done: bug 651015

Great, will write the patch tomorrow.  :-)

Note You need to log in before you can comment on or make changes to this bug.