Stryker: Stryker init creates a temp folder

[simondel]

Running stryker init creates a temp folder that is not removed:

[sharikovvladislav]

Hi!

Do you want to not to create this folder or do you want to delete this folder after test run stryker init finish?

[simondel]

Both are fine by me :) The interesting thing to figure out is why the folder is created in the first place.

[sharikovvladislav]

Ah so it is not planned. Ok, ty.

[sharikovvladislav]

Can you suggest anything so I can debug some npm package? For example I want to debug this problem. I want to somehow install MY OWN COPY of stryker. What are the ways?

I think I can just: npm i /path/to/stryker. Will this work? Is this best practise?

Any other ways?

By the way: I started a conversation in your gitter. I get Stryker is not installed on stryker init right after installing stryker locally.

[nicojs]

By the way: I started a conversation in your gitter. I get Stryker is not installed on stryker init right after installing stryker locally.

That shouldn't be happening. Does it still occur? If so, we should add a new issue.

I think I can just: npm i /path/to/stryker. Will this work? Is this best practise?

Install can be indeed be done with npm i file:/path/to/stryker. You should first build the ts files, but you know that by now 😉 . Since npm5 npm i file:... creates a symlink instead of installing it locally like you might expect. To get the local installation behavior i suggest to use install-local.

Running stryker init creates a temp folder that is not removed:

The init command should not create a .stryker-tmp folder at all. The reason that it does is because the implementation of StrykerTempFolder. Whenever that file is loaded, it immediately creates the folder. This is a very bad practice! Importing the file has a side effect (creation of the folder). This is counter intuitive.

The preferred solution here is to convert all functions in that file to a class. The constructor should than except a string containing the name of the folder to be created (with '.stryker-tmp' as the default). In the constructor, it can create the temp folder on disk. At the beginning of the mutation test method in Stryker, it should create a new instance of the class. All classes using StrykerTempFolder should now use the instance created by Stryker.

[sharikovvladislav]

....Does it still occur?...

I hope I will be able to check this at the evening.

Installation. Will npm link work? Will it be a problem to debug locally installed stryker? As I remember I had added node debug configuration to webstorm and it worked (I debugged grunt-* packages this way) and my own packages.

Whenever that file is loaded, it immediately creates the folder

Exactly. I already realized that too. Thank you for you suggests.

[nicojs]

Will npm link work?

Yes it will.

[sharikovvladislav]

Am I right that all classes wrapped with namespaces are implementations of singleton pattern? Singleton pattern is what I need there. Am I right? ConfigWriterFactory is one of the examples.

So first one who is working with the class will create an instance and next will use already created instance. I will work with that instance through instance() method, right?

[sharikovvladislav]

So the plan is:

• convert StrykerTempFolder to a singleton
• refactor usages of current implementation (in most cases simply by adding .instance())
• refactor units
• look like thats all

[nicojs]

Sounds good. The factory pattern with namespace is pretty complex. You can also do it this way:

class TempDirectory {
  private constructor(){}
  private static _instance: TempDirectory;
  static instance() {
     if(!this._instance){
        this._instance = new TempDirectory();
     }
     return this._instance;
  }
}

Not sure why we didn't use that with our factories.

[sharikovvladislav]

I found that too when I researched about how to implement singletons. I think this is more new approach. Yes it is also much smaller and prettier.

I wanted to implement namespace approach since you used it in stryker. If its ok that I will do same another way - its ok. I will.

[sharikovvladislav]

What about unit tests?

Here is the code:

sandbox.stub(StrykerTempFolder, 'createRandomFolder').returns(workingFolder);
sandbox.stub(StrykerTempFolder, 'copyFile').returns(Promise.resolve({}));
sandbox.stub(StrykerTempFolder, 'writeFile').returns(Promise.resolve({}));

Now I can not use StrykerTempFolder so. I need something like this:

...
sandbox.stub(StrykerTempFolder.instance(), 'createRandomFolder').returns(workingFolder);
...

Or, since there are a lot of usages of that instance I think it is better to declare const let one time, define it in beforeEach and use them in the tests.

What do you think?

[sharikovvladislav]

I started. Here is my first results. I converted bundle of functions to the class. Here is commit: https://github.com/sharikovvladislav/stryker/commit/4628f0fb0306f8eb800948eed7426f8f6be11bfb

Or should I open a PR and we will discuss it in it?

Plan:

• name. is current name is ok?
• refactor codebase
• refactor current units
• perhaps units (do we need units for StrykerTempFolder? there are no ones)

[nicojs]

name. is current name is ok?

Maybe we can call it TempFolder as it makes more sense (we're already in the Stryker repository after all).

On stubbing of StrykerTempFolder.instance(), you can use the mock function as done in the Stryker unit test: https://github.com/stryker-mutator/stryker/blob/master/packages/stryker/test/unit/StrykerSpec.ts#L55

It should look something like this:

import TempFolder from '../../src/utils/TempFolder';
import { mock, Mock } from '../helpers/producers';
import * as sinon from 'sinon';

describe('TempFolder', () => {
  let tempFolderMock: Mock<TempFolder>;
  let sandbox: Sinon.SinonSandbox;
  beforeEach(() => {
    tempFolderMock = mock(TempFolder);
    sandbox = sinon.sandbox.create();
    sandbox.stub(TempFolder, 'instance').returns(tempFolderMock);
    // now you are able to configure return values for tempFolderMock. For example:
    tempFolderMock.writeFile.resolves();
  });
  afterEach(() => sandbox.restore());
});

A Work in Progress PR is indeed easier i think.

[sharikovvladislav]

I started a PR.

@nicojs Ok, I will deal with it and update units.

[sharikovvladislav]

Did you check this? I think this can be closed now.

[simondel]

I just released it and it works like a charm! Thanks for fixing it!