Closed Bug 1411979 Opened 3 years ago Closed 3 years ago

Share the getTempFile function in xpcshell and browser tests

Categories

(Testing :: General, enhancement)

enhancement
Not set

Tracking

(firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

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.
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.
(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
Depends on: 1412282
Duplicate of this bug: 946708
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".
Attachment #8923347 - Flags: review?(mak77) → review+
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68e85782bbca
Share the getTempFile function in xpcshell and browser tests. r=mak
I realized just after pushing that I didn't run xpcshell tests in the trysevrer build. :-(
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.
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
Flags: needinfo?(paolo.mozmail)
Duplicate of this bug: 1093063
Depends on: 1336730
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4d73d768873
Share the getTempFile function in xpcshell and browser tests. r=mak
https://hg.mozilla.org/mozilla-central/rev/f4d73d768873
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1413438
Flags: needinfo?(paolo.mozmail)
Assignee: nobody → paolo.mozmail
You need to log in before you can comment on or make changes to this bug.