Closed Bug 649842 Opened 13 years ago Closed 13 years ago

Intermittent failure in browser/components/sessionstore/test/browser/browser_590268.js | tab 4 has correct uniq2 value - Got 1302731209349.896, expected 1302731209351.6611

Categories

(Firefox :: Session Restore, defect)

Product:
Firefox
Component:
Session Restore
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 6

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

Patch (v1)
956 bytes, patch
zpao
: review+
Details | Diff | Splinter Review 
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.
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.
Attached patch Patch (v1) — — Splinter Review 
Assignee: nobody → ehsan
Status: NEW → ASSIGNED

        Attachment #525852 -
        Flags: review?(paul)

    
    

    
  
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>
Blocks: 603536, 624384

    
    

    
  
(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!  :-)

    
    

    
  
(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.

    
    

    
  
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.

    
    

    
  
Or go for getMonotonicNumber, so we leave the "Rand" word out of the door, for less possible confusion.

    
    

    
  
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 on attachment 525852 [details] [diff] [review]
Patch (v1)

Ah silly me. Thanks for find that.

        Attachment #525852 -
        Flags: review?(paul) → review+

    
    

    
      
  
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/b4be8df347d5
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 6

    
    

    
  
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?

    
    

    
  
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.

    
    

    
  
(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!

    
    

    
  
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.

    
    

    
  
fwiw, Date.now() looks like an unnecessary complication, doesn't add any value to the counter by itself. it's just a bigger number.

    
    

    
  
(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?

    
    

    
  
(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.

    
    

    
  
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.

    
    

    
  
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);

    
    

    
  
Only the http cache. The CORS implementation has a separate cache that holds something else (cached security info).

    
    

    
  
(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.

    
    

    
  
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.

    
    

    
  
(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!

    
    

    
  
(In reply to comment #25)
> Done: bug 651015

Great, will write the patch tomorrow.  :-)
Whiteboard: [orange]

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: