Reduce number of created temp dirs for profiles (closes #2735, closes #2013) #2740
Conversation
|
❌ Tests for the commit a18e1d6 have failed. See details: |
AndreyBelym
changed the title
|
✅ Tests for the commit 95a21d5 have passed. See details: |
|
|
||
| const tempDir = tmp.dirSync({ unsafeCleanup: true }); | ||
| const profileDirName = path.join(tempDir.name, 'Default'); | ||
| const tempDir = await TempDirectory.createDirectory('chrome-profile'); |
There was a problem hiding this comment.
It should be testcafe-chrome-profile to avoid the name conflicts.
There was a problem hiding this comment.
Solved in favor of top-level testcafe-tmp.
| @@ -54,11 +54,9 @@ async function generatePreferences (profileDir, { marionettePort, config }) { | |||
| } | |||
|
|
|||
| export default async function (runtimeInfo) { | |||
| tmp.setGracefulCleanup(); | |||
| const tmpDir = await TempDirectory.createDirectory('firefox-profile'); | |||
There was a problem hiding this comment.
testcafe-firefox-profile
There was a problem hiding this comment.
Solved in favor of top-level testcafe-tmp.
| export default async function (proxyHostName, configString) { | ||
| var config = getConfig(configString); | ||
| var tempProfileDir = !config.userProfile ? await getTempProfile(proxyHostName, config) : null; | ||
| var tempProfileDir = !config.userProfile ? await createTempProfile(proxyHostName, config) : null; |
src/utils/temp-directory.js
Outdated
|
|
||
|
|
||
| // NOTE: mutable for testing purposes | ||
| let TESTCAFE_TMP_DIRS_ROOT = path.join(os.tmpdir(), 'testcafe-tmp'); |
There was a problem hiding this comment.
I guess we can rename testcafe-tmp to testcafe because this folder is already placed in temporary directory.
src/utils/temp-directory.js
Outdated
| this.path = path.join(dirPath, LOCKFILE_NAME); | ||
| } | ||
|
|
||
| _openLockFile () { |
There was a problem hiding this comment.
Unneeded context: rename openLockFile to _open.
src/utils/temp-directory.js
Outdated
|
|
||
| const DEBUG_LOGGER = debug('testcafe:utils:temp-directory'); | ||
|
|
||
| class LockFile { |
There was a problem hiding this comment.
Move LockFile and TempDirectory to separate files.
src/utils/temp-directory.js
Outdated
| } | ||
| } | ||
|
|
||
| _isStaleLockFile () { |
There was a problem hiding this comment.
Unneeded context: rename to _isStale
test/server/util-test.js
Outdated
| const TempDirectory = require('../../lib/utils/temp-directory'); | ||
|
|
||
|
|
||
| const TMP_ROOT = path.join(process.cwd(), '__tmp__'); |
There was a problem hiding this comment.
This constant is used only in `describe('Temp Directory',). So move it.
|
❌ Tests for the commit 15231be have failed. See details: |
|
❌ Tests for the commit 16b8770 have failed. See details: |
|
✅ Tests for the commit 16b8770 have passed. See details: |
|
FPR |
AndreyBelym
changed the title
|
I've tested in on Windows 10, Ubuntu 16.04 VM, macOS 10.13 VM. Additional detached process for killing browsers and cleaning temp dirs works even for #2013 You can grab a debug build here for testing: https://github.com/AndreyBelym/testcafe/releases/tag/build-gh-2735 |
src/utils/temp-directory/index.js
Outdated
| // NOTE: mutable for testing purposes | ||
| let TESTCAFE_TMP_DIRS_ROOT = path.join(os.tmpdir(), 'testcafe'); | ||
|
|
||
| const DEFAULT_NAME_PREFIX = 'tmp'; |
src/utils/temp-directory/index.js
Outdated
| const DEFAULT_NAME_PREFIX = 'tmp'; | ||
|
|
||
| const USED_TEMP_DIRS = {}; | ||
|
|
There was a problem hiding this comment.
Do we really need so many empty string between constants. I wouldn't say it improve the readability.
|
|
||
| DEBUG_LOGGER('Temp directory path: ', this.path); | ||
|
|
||
| await cleanupProcess.init(); |
There was a problem hiding this comment.
await cleanupProcess.init();
await cleanupProcess.addDirectory(this.path);
src/utils/temp-directory/index.js
Outdated
| USED_TEMP_DIRS[this.path] = this; | ||
| } | ||
|
|
||
| disposeSync () { |
There was a problem hiding this comment.
It's not a public method.
src/utils/temp-directory/index.js
Outdated
| } | ||
|
|
||
| // NOTE: for testing purposes | ||
| static get TEMP_DIRECTORIES_ROOT () { |
There was a problem hiding this comment.
Why not
exports.TEMP_DIRECTORIES_ROOT = TEMP_DIRECTORIES_ROOT = ;?
Otherwise a static variable should be initialized in the class.
| } | ||
|
|
||
| init () { | ||
| this.initPromise = this.initPromise |
There was a problem hiding this comment.
How does it work? Is it the workaround for the concurrent init?
There was a problem hiding this comment.
Yes, it's a guard that ensures that init code will be run sequentially when called concurrently. This situation can happen when starting both Chrome and Firefox, or in concurrency mode. Calls to init form a Promise chain, where each next call have to wait until all previous calls are resolved.
| } | ||
| } | ||
|
|
||
| function init () { |
There was a problem hiding this comment.
Why does this function needed, if we already execute it in module context?
| process.on('disconnect', async () => { | ||
| const removePromises = Object | ||
| .keys(DIRECTORIES_TO_CLEANUP) | ||
| .map(dirPath => removeDirectory(dirPath).catch(() => {})); |
There was a problem hiding this comment.
I prefer noop, because our auto-complete settings unfolds () => {} in
() => {
}| addDirectory(message.path); | ||
| await removeDirectory(message.path); | ||
| return; | ||
| default: |
There was a problem hiding this comment.
Why do we use default here, is it for the eslint purposes?
src/utils/temp-directory/lockfile.js
Outdated
|
|
||
| _open ({ force = false } = {}) { | ||
| try { | ||
| const fd = fs.openSync(this.path, force ? 'w' : 'wx'); |
There was a problem hiding this comment.
We can add flag in writeFileSync as well.
|
❌ Tests for the commit bb6d513 have failed. See details: |
|
✅ Tests for the commit bb6d513 have passed. See details: |
…, closes DevExpress#2013) (DevExpress#2740) * Reduce number of created temp dirs for profiles (closes DevExpress#2735) * Add test * Remove temp dirs in a separate process * Fix tests * Fix remarks
No description provided.