1411979 - Share the getTempFile function in xpcshell and browser tests

Status: RESOLVED FIXED

(Testing :: General, enhancement)

Description

[:Paolo Amadini]

We could have a FileTestUtils.getTempFile([optional name]) function.

The reason the function is not shared is that it depends on the current global Assert functions, as well as either do_register_cleanup or registerCleanupFunction. This means we should find a better way to access these in TestUtils-style modules.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Why is the current function not using createNiceUniqueFile?
I honestly don't think we should care about cleanup, it's a tmp file, the OS can do its own job, I assume we may just create a folder based on current timestamp, and use createNiceUniqueFile to ensure to not pick a dupe and that we won't hit the 10k files limit.

Comment 2

[:Paolo Amadini]

(In reply to Marco Bonardo [::mak] from comment #1)
> Why is the current function not using createNiceUniqueFile?

Because that may end up reusing the name of a file that's just been deleted, which creates intermittent failures on Windows under heavy load. Additionally, various situations require that the file does not exist to begin with, and that has never been created before.

> I honestly don't think we should care about cleanup, it's a tmp file

Some tests do create relatively big files, like the background file saver, and we may run the tests many times on the same machine without rebooting. I believe this may happen in automation too. Cleaning up is always a good idea, and may prevent headaches later.

When the utility is shared, though, we may be able to put all files in a single subfolder and only delete the subfolder once, to save I/O calls. This will still be a per-process folder, with xpcshell potentially creating more folders than browser-chrome tests. We should still randomize the folder name to avoid the access denied error when reusing names, as createUnique doesn't handle it, which is understandable as we can't distinguish it from a real issue with an inaccessible folder:

https://dxr.mozilla.org/mozilla-central/rev/a97fb1c39d51a7337b46957bde4273bd308b796a/xpcom/io/nsLocalFileCommon.cpp#79

Comment 4

[:Paolo Amadini]

Attachment: Bug 1411979 - Share the getTempFile function in xpcshell and browser tests.

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

Comment 5

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd9905a04d5cdaf9f0145e6ef3e55a442769baa7

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8923347 [details]
Bug 1411979 - Share the getTempFile function in xpcshell and browser tests.

https://reviewboard.mozilla.org/r/194552/#review199548

As I said on IRC, it looks a little bit legacy since it's synchronous and returns an nsIFile. I assume it's because there are 121 consumers, and part of them rely on this returning an nsIFile, so it would be an expensive job to convert all of them, for a tiny benefit (users could use this code as an example).
Maybe a conversion to OS.File could be something mentorable.

::: testing/modules/FileTestUtils.jsm:52
(Diff revision 1)
> +    gFileCounter++;
> +
> +    // Get a file reference under the temporary directory for this test file.
> +    let file = this._globalTemporaryDirectory.clone();
> +    file.append(leafName);
> +    Assert.ok(!file.exists());

please add a message, even just "Sanity check the temp file doesn't exist".

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68e85782bbca
Share the getTempFile function in xpcshell and browser tests. r=mak

Comment 8

[:Paolo Amadini]

I realized just after pushing that I didn't run xpcshell tests in the trysevrer build. :-(

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a4f33d16b9e
Fix ESLint failure. rs=me

Comment 10

[:Paolo Amadini]

Also, xpcshell tests look good locally, but I haven't run one that is for Windows only.

If that fails, feel free to back out the patches and I'll re-land them tomorrow.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing xpcshell's toolkit/components/jsdownloads/test/unit/test_PrivateTemp.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/bdb364ba3e05ea651d932b0f4c38ab5a7804dee7

Follow-up push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7a4f33d16b9e40788ecdad7cb35582c2abf3ee5c&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=retry&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=140789588&repo=mozilla-inbound

[task 2017-10-30T18:00:10.613Z] 18:00:10     INFO -  toolkit/components/jsdownloads/test/unit/test_PrivateTemp.js | Starting test_private_temp
[task 2017-10-30T18:00:10.614Z] 18:00:10     INFO -  (xpcshell/head.js) | test test_private_temp pending (2)
[task 2017-10-30T18:00:10.615Z] 18:00:10     INFO -  (xpcshell/head.js) | test run_next_test 1 finished (2)
[task 2017-10-30T18:00:10.616Z] 18:00:10     INFO -  TEST-PASS | toolkit/components/jsdownloads/test/unit/test_PrivateTemp.js | test_private_temp - [test_private_temp : 52] Sanity check the temporary file doesn't exist. - true == true
[task 2017-10-30T18:00:10.616Z] 18:00:10  WARNING -  TEST-UNEXPECTED-FAIL | toolkit/components/jsdownloads/test/unit/test_PrivateTemp.js | test_private_temp - [test_private_temp : 23] 493 == 448
[task 2017-10-30T18:00:10.617Z] 18:00:10     INFO -  /builds/worker/workspace/build/tests/xpcshell/tests/toolkit/components/jsdownloads/test/unit/test_PrivateTemp.js:test_private_temp:23
[task 2017-10-30T18:00:10.618Z] 18:00:10     INFO -  exiting test
[task 2017-10-30T18:00:10.618Z] 18:00:10     INFO -  Unexpected exception 2147500036
[task 2017-10-30T18:00:10.619Z] 18:00:10     INFO -  undefined
[task 2017-10-30T18:00:10.619Z] 18:00:10     INFO -  exiting test

Comment 13

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8696c6066cb91836b7b65bbe917fca9ce4c76015

Comment 14

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b75770f67423b97dadc211dda276cb17128ad5f

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4d73d768873
Share the getTempFile function in xpcshell and browser tests. r=mak

Comment 16

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/f4d73d768873