Share the getTempFile function in xpcshell and browser tests

RESOLVED FIXED in Firefox 58

Status

Testing
General
RESOLVED FIXED
29 days ago
24 days ago

People

(Reporter: Paolo, Unassigned)

Tracking

(Depends on: 1 bug)

Trunk
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

29 days ago
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.
(Reporter)

Comment 2

29 days ago
(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
(Reporter)

Updated

29 days ago
Depends on: 1412282
(Reporter)

Updated

26 days ago
Duplicate of this bug: 946708
Comment hidden (mozreview-request)
(Reporter)

Comment 5

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

Comment 6

25 days ago
mozreview-review
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+

Comment 7

25 days ago
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
(Reporter)

Comment 8

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

Comment 9

25 days ago
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a4f33d16b9e
Fix ESLint failure. rs=me
(Reporter)

Comment 10

25 days ago
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)
(Reporter)

Updated

25 days ago
Duplicate of this bug: 1093063
(Reporter)

Updated

24 days ago
Depends on: 1336730
(Reporter)

Comment 13

24 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8696c6066cb91836b7b65bbe917fca9ce4c76015
(Reporter)

Comment 14

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

Comment 15

24 days ago
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

Comment 16

24 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f4d73d768873
Status: NEW → RESOLVED
Last Resolved: 24 days ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(Reporter)

Updated

24 days ago
Depends on: 1413438
(Reporter)

Updated

24 days ago
Flags: needinfo?(paolo.mozmail)
You need to log in before you can comment on or make changes to this bug.