feature(stryker): #336 Configurable tmp folder location #475

avassem85 · 2017-11-26T18:43:31Z

No description provided.

…used and replaced by TempFolder

nicojs

Looks good! I still have some remarks.

Also: how do we prevent a user from using an absolute path? Using an absolute path (for example /tmp/dir/) breaks mocha tests, however it would not break karma tests.

Do we want to prevent users from configuring an absolute path? @simondel maybe you have an opion on this?

nicojs · 2017-11-27T08:23:44Z

packages/stryker/README.md

+
+#### Temporary folder  
+**Command line:** `--tempFolder .stryker-tmp`    
+**Config file:** `logLevel: '.stryker-tmp'`


Still refers to logLevel here

nicojs · 2017-11-27T08:25:06Z

packages/stryker/src/StrykerCli.ts

@@ -47,6 +47,7 @@ export default class StrykerCli {
      .option('--timeoutFactor <number>', 'Tweak the standard deviation relative to the normal test run of a mutated test', parseFloat)
      .option('--maxConcurrentTestRunners <n>', 'Set the number of max concurrent test runner to spawn (default: cpuCount)', parseInt)
      .option('--logLevel <level>', 'Set the log4js log level. Possible values: fatal, error, warn, info, debug, trace, all and off. Default is "info"')
+      .option('--tempFolder <name>', 'Set name of the folder that contains temporary files')


Should we call it tempFolder or tempDir (or tempDirectory). I think tempDir is more clear, we should always refer to directory instead of folder.

I think tempDir is better. But if we change it we should also change the TempFolder class.

Renamed to tempDir

nicojs · 2017-11-27T08:27:16Z

packages/stryker/src/utils/TempFolder.ts

@@ -15,6 +15,7 @@ export class TempFolder {
  initialize(tempDirName = '.stryker-tmp') {


This variable tempDirName should not have a default value, as this is a duplicate from the Config class.

nicojs · 2017-11-27T08:27:37Z

packages/stryker/test/unit/utils/TempFolderSpec.ts


        expect(fileUtils.deleteDir).to.have.been.calledWith(
          tempFolderInstance.baseTempFolder
        );

-        result.then(data => expect(data).equals('delResolveStub'));
+        expect(result).equals('delResolveStub');


This is better indeed 👍

simondel · 2017-11-27T08:43:29Z

packages/stryker/README.md

+**Default value:** `.stryker-tmp`  
+**Mandatory**: no 
+**Description:**  
+ Set name of the folder that contains temporary files


I think it's smart to mention that stryker will attempt to delete this folder after it's done.

simondel · 2017-11-27T08:44:15Z

packages/stryker/src/StrykerCli.ts

@@ -47,6 +47,7 @@ export default class StrykerCli {
      .option('--timeoutFactor <number>', 'Tweak the standard deviation relative to the normal test run of a mutated test', parseFloat)
      .option('--maxConcurrentTestRunners <n>', 'Set the number of max concurrent test runner to spawn (default: cpuCount)', parseInt)
      .option('--logLevel <level>', 'Set the log4js log level. Possible values: fatal, error, warn, info, debug, trace, all and off. Default is "info"')
+      .option('--tempFolder <name>', 'Set name of the folder that contains temporary files')


I think tempDir is better. But if we change it we should also change the TempFolder class.

simondel · 2017-11-27T08:45:26Z

@nicojs Why would an absolute path break mocha?

avassem85 · 2017-11-27T20:41:47Z

@simondel Because mocha uses Require that search node_modules up in the directories of the application. When the temp folder is not within the root of the application is will fail.

nicojs

Some small cosmetic remarks.

nicojs · 2017-11-28T06:33:11Z

packages/stryker-api/src/config/Config.ts

@@ -19,6 +19,7 @@ export default class Config implements StrykerOptions {
  mutator: string = 'es5';
  transpilers: string[] = [];
  maxConcurrentTestRunners: number = Infinity;
+  tempFolder: string = '.stryker-tmp';


This also needs to be renamed to tempDir

nicojs · 2017-11-28T06:34:29Z

packages/stryker/src/utils/StrykerTempFolder.ts

@@ -1,74 +0,0 @@
-import * as fs from 'mz/fs';


Good catch! Removing code makes me happy

nicojs · 2017-11-28T06:48:11Z

packages/stryker/README.md

+**Default value:** `.stryker-tmp`  
+**Mandatory**: no 
+**Description:**  
+ Set name of the folder that contains temporary files


Let's make this a little more descriptive. Something like:

Set the name of the directory that is used by Stryker as a working directory. This directory is used to keep copies of your files that are used by Stryker to mutate your code safely. This directory will be cleaned after a successful run. Warning! If you configure an absolute path (for example /tmp/stryker), node-based test runners like mocha will fail, because your node_modules cannot be loaded. Use with caution.

nicojs · 2017-11-28T06:49:18Z

packages/stryker/src/StrykerCli.ts

@@ -47,6 +47,7 @@ export default class StrykerCli {
      .option('--timeoutFactor <number>', 'Tweak the standard deviation relative to the normal test run of a mutated test', parseFloat)
      .option('--maxConcurrentTestRunners <n>', 'Set the number of max concurrent test runner to spawn (default: cpuCount)', parseInt)
      .option('--logLevel <level>', 'Set the log4js log level. Possible values: fatal, error, warn, info, debug, trace, all and off. Default is "info"')
+      .option('--tempDir <name>', 'Set name of the folder that contains temporary files. Stryker will try to remove the content when the tests ran')


Let's also update this one. Something like:

"Set the name of the directory that is used by Stryker as a working directory. This directory will be cleaned after a successful run. "

nicojs · 2017-11-28T10:31:19Z

Only the deepest directory is deleted. Is this acceptable?

Also it will be deleted regardless of any other data in the directory. So if you would point it to '/', your entire computer will be wiped clean after a successful run.

avassem85 · 2017-11-28T10:52:21Z

@nicojs

If you delete more than the deepest directory you could end up deleting to much (temporary locations for other tools)
You can't accidentally delete root(/) because the directory is always relative from the root of your project

nicojs · 2017-11-28T13:24:18Z

You can't accidentally delete root(/) because the directory is always relative from the root of your project

Well... i could incidentally use tempDir: '../../../../../'

avassem85 · 2017-11-28T16:48:31Z

@nicojs true, but what do you want to do about it?

nicojs · 2017-12-06T07:00:51Z

@nicojs true, but what do you want to do about it?

Well.. Maybe we could clean the directory first. Remove all sandbox sub directories. If the directory is empty after that, we can remove it safely.

To remove all sandbox sub directories, we can just use rimraf and give it the pattern: '.stryker-tmp/sandbox*'. Also i've noticed that we are creating a temp folder within the temp folder. I don't know why. I'm talking about this line: https://github.com/stryker-mutator/stryker/blob/24699e16e9c3c8ea77ecb52f4f1b6a8729ac5306/packages/stryker/src/utils/TempDir.ts#L17

I think we can safely remove that.

nicojs · 2018-05-11T07:15:16Z

This PR is stale. Closing for now, please create a new PR.

brunoqueiros · 2019-07-12T15:15:02Z

any plans to implement the configurable tempFolder? @nicojs

nicojs · 2019-07-15T11:00:59Z

We've got lot of plans 😅

This is pretty easy to solve if we can simply give it another name. If you want support for it inside your OS temp folder it will take some more time. What would you prefer?

brunoqueiros · 2019-07-15T19:33:03Z

I was thinking something similar with this PR where you just specify another name inside the project itself. —tempDir=.another-path-tmp. The main reason for that is to be able to run multiple stryker run commands in parallel.

Do you need help with it?

nicojs · 2019-07-15T19:57:09Z

Help is always welcome!

The best solution would be to make TempFolder injectable and implement the Disposable interface from typed-inject. That way we can also remove the explicit call to TempFolder.instance().clean (since it will be injectable).

If you need help setting up the dependency injection, please let me know.

Inside the TempFolder you can let the options be injected. We add an option as you propose.

We should also rename TempFolder to TempDir or TemporaryDirectory. Calling directories folders is kind of a pet peeve of mine 😅 .

brunoqueiros · 2019-07-16T15:04:04Z

i created a PR #1644, could you check if I am going in the right track regarding the DI.

avassem85 added 2 commits November 26, 2017 17:00

chore(StrykerTempFolder): Remove StrykerTempFolder because it is not …

dd89d01

…used and replaced by TempFolder

feature(stryker): #336 Configurable tmp folder location

bb10a1b

avassem85 requested a review from simondel November 26, 2017 18:43

avassem85 mentioned this pull request Nov 26, 2017

Configurable tmp folder location #336

Closed

nicojs requested changes Nov 27, 2017

View reviewed changes

simondel requested changes Nov 27, 2017

View reviewed changes

Fix review comments

180d8cd

avassem85 self-assigned this Nov 27, 2017

nicojs requested changes Nov 28, 2017

View reviewed changes

avassem85 and others added 3 commits November 28, 2017 08:43

Fix some minor review comments

62d9950

Fix name of tempDir configuration

4f9a7f9

Merge branch 'master' into config-temp-dir

24699e1

simondel added this to Review in Backlog Jan 10, 2018

simondel added the 🔎 Needs review label Apr 26, 2018

nicojs closed this May 11, 2018

Backlog automation moved this from Review to Done May 11, 2018

ghost removed the 🔎 Needs review label May 11, 2018

nicojs deleted the config-temp-dir branch July 4, 2018 06:29

brunoqueiros mentioned this pull request Jul 16, 2019

feature(stryker): #336 Configurable tmp folder location #1644

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(stryker): #336 Configurable tmp folder location #475

feature(stryker): #336 Configurable tmp folder location #475

avassem85 commented Nov 26, 2017

nicojs left a comment

nicojs Nov 27, 2017

avassem85 Nov 27, 2017

nicojs Nov 27, 2017

simondel Nov 27, 2017

avassem85 Nov 27, 2017

nicojs Nov 27, 2017

nicojs Nov 27, 2017

simondel Nov 27, 2017

avassem85 Nov 27, 2017

simondel Nov 27, 2017

simondel commented Nov 27, 2017

avassem85 commented Nov 27, 2017

nicojs left a comment

nicojs Nov 28, 2017

nicojs Nov 28, 2017

nicojs Nov 28, 2017

nicojs Nov 28, 2017

nicojs commented Nov 28, 2017

avassem85 commented Nov 28, 2017

nicojs commented Nov 28, 2017

avassem85 commented Nov 28, 2017

nicojs commented Dec 6, 2017

nicojs commented May 11, 2018

brunoqueiros commented Jul 12, 2019 •

edited

nicojs commented Jul 15, 2019

brunoqueiros commented Jul 15, 2019

nicojs commented Jul 15, 2019

brunoqueiros commented Jul 16, 2019

		@@ -15,6 +15,7 @@ export class TempFolder {
		initialize(tempDirName = '.stryker-tmp') {

feature(stryker): #336 Configurable tmp folder location #475

feature(stryker): #336 Configurable tmp folder location #475

Conversation

avassem85 commented Nov 26, 2017

nicojs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simondel commented Nov 27, 2017

avassem85 commented Nov 27, 2017

nicojs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicojs commented Nov 28, 2017

avassem85 commented Nov 28, 2017

nicojs commented Nov 28, 2017

avassem85 commented Nov 28, 2017

nicojs commented Dec 6, 2017

nicojs commented May 11, 2018

brunoqueiros commented Jul 12, 2019 • edited

nicojs commented Jul 15, 2019

brunoqueiros commented Jul 15, 2019

nicojs commented Jul 15, 2019

brunoqueiros commented Jul 16, 2019

brunoqueiros commented Jul 12, 2019 •

edited