584283 - Make XPCShell more compatible with testing the disk cache.

Status: NEW

(Core :: Networking: Cache, defect, P5)

Description

[Byron Milligan [:byronm]]

Attachment: Defaults to having disk cache turned on for xpcshell tests.

Currently, the xpcshell has the default behavior that the disk cache is turned off. In order to use the disk cache when running xpcshell tests, one must set the environment variable "NECKO_DEV_ENABLE_DISK_CACHE". See Bug 485230 for details. Ehsan, this also ties in to your Bug 460430. I think 485230 actually resolved 460430, but I thought you might like to confirm that.

This patch proposes the following, which are some ideas that Jason Duell and I came up with: 

1)That we invert NECK_DEV_ENABLE_DISK_CACHE such that disk cache is enabled by  
  default, and can be explicitly disabled by setting an equivalent (but 
  renamed) environment variable. I think this makes sense, because it seems  
  more intuitive to expect that the disk cache be turned on than off. Further, 
  we have tests that exercise the cache, and we would ideally have them 
  exercising the disk cache code as well as the memory cache code.

2)
That it be possible to specify a path to the specific directory that should be  used for the disk cache. Because xpcshell apparently does not have access  to the standard profile directory, a directory is created inside the object directory using the NS_XPCOM_CURRENT_PROCESS_DIR macro. This macro represents the directory in which the xpcshell test was started from. On my Linux environment, the path is [obj-dir]/dist/bin/Cache.
  
3)That it be possible to specify if the disk cache data should persist/be blown 
  away across multiple xpcshell sessions. By default, the behavior is that the 
  directory persists.
  
This initial draft that I posted solves issue 1. I will happily tackle 2 and 3 if we agree they are a good idea; I have to investigate our nsIFile interface in order to do 2. 3 is currently fixable by manually removing the directory in between runs. 

Let me know any thoughts. Thanks!

Comment 1

[Benjamin Smedberg]

I don't think you need to mess with NECKO_DEV_ENABLE_DISK_CACHE at all. In the tests you want to use cacheing, just set up a profile directory by hand (using a directory provider) and clean up the directory when you're done. You shouldn't need any changes to xpcshell at all.

Comment 2

[(no longer active)]

I agree with Benjamin in comment 1.

Comment 3

[(not currently active) Ted Mielczarek]

do_get_profile() in the xpcshell test harness gives you a profile directory (in a temp dir) + a directory provider that returns that dir for ProfD.

Comment 4

[Jason Duell]

Isn't it poor test coverage to disable the disk cache for all of xpcshell tests (except where manually specified otherwise), and I assume also all of mochitest?  It's certainly not the common case that people have the disk cache off.

Comment 5

[(no longer active)]

(In reply to comment #4)
> Isn't it poor test coverage to disable the disk cache for all of xpcshell tests
> (except where manually specified otherwise), and I assume also all of
> mochitest?  It's certainly not the common case that people have the disk cache
> off.

For xpcshell tests, not at all.  We don't initialize anything unless it's needed in xpcshell environment, so I don't see why the disk cache should be any different.

However, for mochitests, I don't believe that we disable the disk cache (http://mxr.mozilla.org/mozilla-central/source/testing/tools/profiles/createTestingProfile.py seems to confirm this), but if we do, we should enable it *right now*!

Comment 6

[(not currently active) Ted Mielczarek]

Mochitests are run in a browser and the cache should work fine. We just use xpcshell to run the httpd there.

Comment 7

[Byron Milligan [:byronm]]

(In reply to comment #1)
> I don't think you need to mess with NECKO_DEV_ENABLE_DISK_CACHE at all. In the
> tests you want to use cacheing, just set up a profile directory by hand (using
> a directory provider) and clean up the directory when you're done. You
> shouldn't need any changes to xpcshell at all.

To clarify: Does this mean that I should be able to enable the disk cache by setting up a profile, and that this is better than setting NECKO_DEV_ENABLE in my environment?

Comment 8

[Jason Duell]

Byron:  sounds like we can get rid of NECKO_DEV_ENABLE_DISK_CACHE in the code, and instead create a enable_cache.js file that tests that want the disk cache on can include, with cache_enable and cache_cleanup functions that create/delete a profile directory.  If your JS-fu is good enough, you might even be able to make the profile get automatically deleted w/o having to explicitly call a cleanup function--that'd be nice.

But I'd just use NECKO_DEV_ENABLE for now, and only implement this if/when you find cycles for it.  Not high-priority.

Comment 9

[(not currently active) Ted Mielczarek]

Using do_get_profile() gets you a profile in a temp directory that's automatically cleaned up by the Python harness.

Comment 10

[Byron Milligan [:byronm]]

(In reply to comment #9)
> Using do_get_profile() gets you a profile in a temp directory that's
> automatically cleaned up by the Python harness.

Ok. So are you saying that if I make an xpcshell test that looks like this: 

run_test() {
 do_get_profile();
 call_code_that_uses_profile();
}

Things should just work? I do not have to use any sort of directory provider as bsmedberg suggested in Comment 1? And that since I have a profile, which defaults to having the disk cache enabled in all.js, the disk cache should be turned on as well? 

The work prompting this thread in the first place can be found in the attachment to Bug 585777. Basically, it is an xpcshell test that takes a list of URL's, and calculates things like the hit rate on the cache, number of evictions, etc. I just want to make sure that I have everything configured correctly, so that I can trust the numbers it gives me.

Thanks, and sorry if this is obvious.

Comment 11

[(not currently active) Ted Mielczarek]

That's correct. do_get_profile installs a directory provider for you:
http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/head.js#489

Comment 12

[Byron Milligan [:byronm]]

(In reply to comment #11)
> That's correct. do_get_profile installs a directory provider for you:
> http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/head.js#489

Thank you Ted. And just to be sure, even though the profile will be created/destroyed with each execution of the xpcshell test, it should be the case that the same cache directory persists, and is used across executions of thetest, right? This would make since to me, since the cache directory lives outside of the profile.

Comment 13

[Byron Milligan [:byronm]]

Actually, upon further investigation, it looks like the Cache directory lives in /tmp/firefox/xpcshellprofile/Cache. And that each run of xpcshell overwrites this, since the profile gets created/deleted with each run.

This behavior differs from if I set NECKO_DEV_ENABLE_DISK_CACHE, which puts the cache directory in /dist/bin of the objdir. But I have not tried doing this in combination with do_get_profile. I'll see what happens.

Does this seem right?

Comment 14

[(not currently active) Ted Mielczarek]

Putting data in dist/bin would be a terrible idea, since you'd have variable behavior between test runs. The profile directory that do_get_profile uses is deleted after each individual test_*.js file is run, so there's no data leakage between tests.

Comment 15

[Jason Duell]

OK, so let's just rip out NECKO_DEV_ENABLE_DISK_CACHE and use do_get_profile() when we want caching.

Ted: Am I correct in thinking that if we call do_get_profile in a head_*.js file, a single profile is created for the duration of all the tests in that directory?  Or are head_ files loaded for each separate test file?

Either way, we can create a head_enable_caching.js file, have it turn caching on by default (I still think it makes sense to turn on caching by default for necko xpcshell tests), and then maybe provide a function that specific tests can use to turn off caching (by setting the appropriate prefs).

Comment 16

[(not currently active) Ted Mielczarek]

(In reply to comment #15)
> Ted: Am I correct in thinking that if we call do_get_profile in a head_*.js
> file, a single profile is created for the duration of all the tests in that
> directory?  Or are head_ files loaded for each separate test file?

Each test file is run in a separate xpcshell instance, with the head_*.js files loaded in that instance, so they'll each get their own fresh profile.

Comment 17

[Jason Duell]

> Each test file is run in a separate xpcshell instance

Great.  So it should be easy to write a function in head_enable_caching.js that disables caching for a particular test if needed/wanted.  Let's make sure to document this in the MDC "running xpcshell tests" page.  It took me an embarassingly long time once to figure out that I wasn't seeing network traffic while debugging because the cache was on :)

Comment 18

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258