Closed Bug 1411979 Opened 3 years ago Closed 3 years ago
Share the get
Temp File function in xpcshell and browser tests
59 bytes, text/x-review-board-request
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
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 firstname.lastname@example.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. :-(
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7a4f33d16b9e Fix ESLint failure. rs=me
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
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/f4d73d768873 Share the getTempFile function in xpcshell and browser tests. r=mak
You need to log in before you can comment on or make changes to this bug.