Bugfix: Stryker init creates a temp folder #361

sharikovvladislav · 2017-08-28T07:36:12Z

Impl of #320

TODO:

change name of class to TempFolder
refactor codebase to the new TempFolder class
refactor unit tests
perhaps: units

sharikovvladislav · 2017-08-28T07:37:35Z

Is TODO is ok, should I do something else?
If I understand correctly there are no units for current StrykerTempFolder module. Should I add tests? What kind of tests do we need?
do you have this problem? http://i.imgur.com/gowExH0.png. I can't see @types/jasmine. If I add it, everything becomes ok.

Also:

is it ok that I ask you this questions? :) should I do everything alone maybe? I think it is much faster if I ask. It is much worse when I do something and do it again after you said I did bad.
can I ask in gitter then?

PS: some time build will be red.

nicojs · 2017-08-28T08:05:12Z

Todo looks fine indeed. Thanks for picking this up!

is it ok that I ask you this questions? :) should I do everything alone maybe? I think it is much faster if I ask. It is much worse when I do something and do it again after you said I did bad.

That's fine, if your ok with waiting up to a day on answers. Gitter can be faster at times.

do you have this problem? http://i.imgur.com/gowExH0.png. I can't see @types/jasmine. If I add it, everything becomes ok.

We're not using jasmine as a test framework, instead we're using mocha (test framework) and chai (assertion library). It looks like jasmine, but it is not.

If I understand correctly there are no units for current StrykerTempFolder module. Should I add tests? What kind of tests do we need?

True and yes i would like unit tests, but not strictly necessary. We can do that some other time.

sharikovvladislav · 2017-08-28T08:23:17Z

Ok, lets continue with questions in gitter.

sharikovvladislav · 2017-08-29T05:03:00Z

Any ideas ? https://gitter.im/stryker-mutator/stryker?at=59a40add66c1c7c477ecba5c

I put back old name and tests became green. I am not sure. Is this correct?

nicojs · 2017-08-29T07:03:48Z

indeed, the integration tests of the stryker-html-reporter contain a snapshot of the events stryker reporter callbacks, in order to test the integration of stryker-html-reporter without the need to run stryker each time. So it's loosely coupled.

I see you changed the expect back to it's original name, i think that's fine. There is no technical reason to keep the events in the stryker-html-reporter in sync with current stryker file names. That would be a waste of time

nicojs · 2017-08-29T07:12:01Z

For the unit tests, be sure to mock away all the interactions with the file system, as we want our unit tests to be able to run in complete isolation, without any side effects. That is the difference with integration tests, as there we usually test with side effects. Side effects here is creating files stuff.

In this case, you should stub mkdirp.sync, fs.writeFile and deleteDir.

sharikovvladislav · 2017-08-29T07:38:42Z

Okay! So one more item to do is to create new unit tests for refactored class, right?

I did all other work? Am I right? Is everything correct? I mean: refactoring of the class to fix the bug.

nicojs · 2017-08-29T08:33:16Z

I did all other work? Am I right? Is everything correct? I mean: refactoring of the class to fix the bug.

Yes looks good! indeed the only thing remaining is add unit tests.

sharikovvladislav · 2017-08-29T08:35:09Z

Ok, cool. I will research how to do this.

nicojs

Yes looks good! indeed the only thing remaining is add unit tests.

nicojs · 2017-08-30T06:21:06Z

It might be better to not create the temp folder in the constructor. Instead, we should have an initialize(folderName: string) method. This creates the temp folder. This will make it better testable and we are able to implement #336 easier in a later stage. The idea is that the initialize function is called once right before the initial test run, from the Stryker class. Right now it would always provide .stryker-tmp as a default, later we can change it to a config setting. If initialize is not called, we should not do anything different, just let the code break.

Making methods dependent on each other like this is a bad practise, but the only way around that would be to not use a Singleton, instead create an instance of a TempFolder and pass that along. You can do that as well if you choose.

The TempFolder is only used during mutation testing. It is the place were we store the sandboxes on disk. A sandbox is a copy of all your files with (possibly) a mutation in one of them. It is where the actual mutation testing occurs. I started a discussion on #366 to see if we can do in-memory testing. If that happens we can delete the TempFolder all together as its not needed.

So the TempFolder is only used for sandboxes. If a class wants to log something it should create an instance of the log4js Logger and start using that. Later we can add logging to a log file using log4js functionality. For now, don't mind about log files.

sharikovvladislav · 2017-08-30T21:02:08Z

Okay, I updated the code. What do you think? I added initialize method. I will update tests if you think this is ok. https://github.com/stryker-mutator/stryker/pull/361/files#diff-3b57d820a130bce2b8a1d1428567eb28R13

sharikovvladislav · 2017-08-31T18:05:00Z

Any thoughts?

nicojs

Already looks good. Had some small remarks on the unit tests.

nicojs · 2017-09-01T06:28:07Z

packages/stryker/test/unit/MutatorOrchestratorSpec.ts


  beforeEach(() => {
    sandbox = sinon.sandbox.create();
-    sandbox.stub(StrykerTempFolder, 'writeFile');
+    tempFolderMock = mock(TempFolder);


Looks good! Awesome this works, even with a class with a private constructor.

Nope. Looks like it does not work

Ah true. Damn, i think this is a limitation of typescript. It will work if you you change the mock:

export function mock<T>(constructorFn: { new(...args: any[]): T; }): Mock<T>; export function mock<T>(constructorFn: any): Mock<T>; export function mock<T>(constructorFn: any): Mock<T> { return sinon.createStubInstance(constructorFn) as Mock<T>; }

And than use it like this: mock<TempFolder>(TempFolder);

Hm. Now I am getting this: https://travis-ci.org/stryker-mutator/stryker/jobs/270886353#L1771. Still can not get what is the problem.

nicojs · 2017-09-01T06:29:02Z

packages/stryker/src/utils/TempFolder.ts

+
+const log = log4js.getLogger('TempFolder');
+
+export class TempFolder {


We should add a private constructor() { } in order to prevent anyone from creating an instance them selves.

nicojs · 2017-09-01T06:29:46Z

packages/stryker/test/unit/SandboxSpec.ts

+    tempFolderMock = mock(TempFolder);
+    sandbox.stub(TempFolder, 'instance').returns(tempFolderMock);
+    tempFolderMock.createRandomFolder.returns(workingFolder);
+    tempFolderMock.copyFile.resolves({});


Looks great! It is indeed better to use stub.resolves instead of stub.returns(Promise.resolve())

nicojs · 2017-09-01T06:32:51Z

packages/stryker/test/unit/TempFolderSpec.ts

+  });
+  afterEach(() => sandbox.restore());
+
+  it('TempFolder is presented', () => {


What exactly are you testing here? This test can never fail, right? If one of the methods is missing, we would get a compile error in our production code. The benefit of using typescript, is that we don't need tests like these. You can remove it 👍

In this test, your basically verifying that TypeScript is transpiled correctly.

This test will fail if somebody remove/rename this class by accident . This test will force him think about it again. Do you think it is not needed?

No. That's why we use typescript. Having the test won't hurt of course.

nicojs · 2017-09-01T06:33:36Z

packages/stryker/test/unit/TempFolderSpec.ts

+import * as mkdirp from 'mkdirp';
+import * as fs from 'mz/fs';
+
+describe.only('TempFolder', () => {


Remove the only here 😺

Yep, I will. Tests are not ready atm 😿 I will do it of course when I finish to debug my units.

nicojs · 2017-09-01T06:34:54Z

packages/stryker/test/unit/TempFolderSpec.ts

+      const mockCwd = process.cwd() + '/some/dir';
+      sandbox.stub(mkdirp, 'sync');
+      sandbox.stub(process, 'cwd').returns(mockCwd);
+      sandbox.stub(TempFolder.instance(), 'random').returns('rand');


I think it's better to move this stubbing to the top before each. Putting it right here clutters the tests a bit. You can still do the cwdStub.returns(mockCwd) and randomStub.returns('rand') here.

nicojs · 2017-09-01T06:35:53Z

packages/stryker/test/unit/TempFolderSpec.ts

+    });
+  });
+
+  xdescribe('constructor logic', () => {


Should this still be removed? Or changed into the testing of initialize()?

Yes it should be changed. Tests are not ready. I will update them since current code is ok (as I am getting from your review). I just wanted to know if TempFolder.ts code is correct.

I removed it.

sharikovvladislav · 2017-09-01T06:48:41Z

Uhm. I was asking about is current implementation is ok (not tests). Test are not ready at all. Actually, I am ready to put them into a trash.

Since everything except units looks good (as I understood from your review) I am going to fix units now.

sharikovvladislav · 2017-09-01T06:57:47Z

❓ @nicojs Do you think TempFolder should generate an exception if some method was called, but initialize() were not called?

…-320/temp-directory-creates-on-file-import

sharikovvladislav · 2017-09-01T07:22:17Z

packages/stryker/src/utils/TempFolder.ts

+   * Deletes the Stryker-temp folder
+   */
+  clean() {
+    log.debug(`Cleaning stryker temp folder ${this.baseTempFolder}`);


Is word "clean" in that context"? As I understand "clean" means "clean contents of the directory". But purpose of that function is to remove directory completely.

Are you sure this is correct? Perhaps "remove" will be better since it will say what really happens.

Yeah, remove would be better. Yes the purpose is to remove the dir completely.

Is word "clean" in that context"?

Sorry I wanted to say "is it correct in that context"?

OK! I will fix.

sharikovvladislav · 2017-09-01T07:53:04Z

How do you see unit test for copyFile?:

  copyFile(fromFilename: string, toFilename: string, instrumenter: NodeJS.ReadWriteStream | null): Promise<void> {
    return new Promise<void>((resolve, reject) => {
      let readStream: NodeJS.ReadableStream = fs.createReadStream(fromFilename, { encoding: 'utf8' });
      let writeStream = fs.createWriteStream(toFilename);
      readStream.on('error', reject);
      writeStream.on('error', reject);
      if (instrumenter) {
        readStream = readStream.pipe(instrumenter);
      }
      readStream.pipe(writeStream);
      readStream.on('end', () => resolve());
    });
  }

Do you have any examples of units for same things?

nicojs · 2017-09-01T10:00:15Z

❓ @nicojs Do you think TempFolder should generate an exception if some method was called, but initialize() were not called?

Well... maybe that's smart. It communicates clear intent (but you will need extra unit tests).

How do you see unit test for copyFile?:

You can use the 'test/helpers/streamHelpers.ts'. It has helpers to convert a stream to a string and back. But you can also skip this unit test, as we're not going to use it anymore in the future when we have transpiler support (coming with stryker-typescript).

sharikovvladislav · 2017-09-01T13:44:56Z

but you will need extra unit tests

How this unit test will look like if one and first initialize call will influence all tests. Perhaps I can reset state to uninitialised manually after each test. I will not initialize it before each test but only for specific.

nicojs · 2017-09-02T08:38:11Z

packages/stryker/src/Stryker.ts

@@ -81,7 +81,8 @@ export default class Stryker {
      this.reporter.onScoreCalculated(score);
      ScoreResultCalculator.determineExitCode(score, this.config.thresholds);
      await this.wrapUpReporter();
-      await StrykerTempFolder.clean();
+      TempFolder.instance().initialize();


I think this one should be moved up right? Right before the initialTestRun?

nicojs · 2017-09-02T08:41:37Z

Perhaps I can reset state to uninitialized manually after each test.

I will not initialize it before each test but only for specific.

That sounds good, but i would do it in a beforeEach just to be sure. So one beforeEach in a describe('when temp folder is initialized') and one in a describe('when temp folder is not initialized')

The private constructor seems to work now. I think just moving the call to the initialization

sharikovvladislav · 2017-09-02T10:34:51Z

I added tests. Here is test run output:

    createRandomFolder
      when temp folder is initialized
        ✓ should create dir with correct path
      when temp folder is not initialized
        ✓ should throw error
    clean
      when temp folder is initialized
        ✓ should call deleteDir fileApi
      when temp folder is not initialized
        ✓ should throw error
    writeFile
      ✓ should call fs.writeFile
  6 passing (17ms)

What do you think?
I did not add initialise call check to writeFile since this methods does not work with baseTempFolder. Why they are in TempFolder?
Did you try prettier? (:

sharikovvladislav · 2017-09-02T10:38:44Z

Also final check is passed: tmp dir is not created on stryker init but it is created on stryker run.

nicojs · 2017-09-03T13:47:42Z

Thanks!

* master: fix(stryker-init): Stryker init won't create temp folder (stryker-mutator#361) Publish fix(stryker task): Guard against clearing the require cache (stryker-mutator#362) (stryker-mutator#369) chore(build): Make build ts 2.5 compatible (stryker-mutator#368)

bugfix: refactor StrykerTempFolder to a class

4628f0f

sharikovvladislav mentioned this pull request Aug 28, 2017

Configurable tmp folder location #336

Closed

sharikovvladislav added 3 commits August 28, 2017 14:45

refactor: refactor StykerTempFolder usage (tests & code)

fbf1fa9

refactor: refactor rename StykerTempFolder to TempFolder

8e9c77e

refactor: put back old name

f1e30cb

nicojs approved these changes Aug 29, 2017

View reviewed changes

sharikovvladislav added 2 commits August 29, 2017 23:33

refactor: add unit tests for TempFolder

1816334

refactor: add basic tests and test stubs

108dc6a

refactor: add intialize method

3b5829d

nicojs requested changes Sep 1, 2017

View reviewed changes

sharikovvladislav added 4 commits September 1, 2017 10:00

Merge remote-tracking branch 'original-repo/master' into bugfix/issue…

de9f6bc

…-320/temp-directory-creates-on-file-import

refactor: some fixes in tests

da4676b

refactor: add private constructor

afb737a

style: extra line

c36f17a

sharikovvladislav commented Sep 1, 2017

View reviewed changes

test: add tests for clean and initialize

75a229a

style: fix coding style

a87b410

test: remove only

4c63b27

sharikovvladislav added 2 commits September 1, 2017 18:55

test: fix log messages

19fbabb

test: try fix private constructor

daeca9a

nicojs reviewed Sep 2, 2017

View reviewed changes

sharikovvladislav added 2 commits September 2, 2017 13:17

refactor: move initialize to the start of process

42c57b9

refactor: add check in initialized was not called

c5f3d6f

nicojs merged commit a4333c9 into stryker-mutator:master Sep 3, 2017

sharikovvladislav deleted the bugfix/issue-320/temp-directory-creates-on-file-import branch September 3, 2017 14:37


		const log = log4js.getLogger('TempFolder');

		export class TempFolder {

Bugfix: Stryker init creates a temp folder #361

Bugfix: Stryker init creates a temp folder #361

Conversation

sharikovvladislav commented Aug 28, 2017 • edited

sharikovvladislav commented Aug 28, 2017 • edited

nicojs commented Aug 28, 2017

sharikovvladislav commented Aug 28, 2017

sharikovvladislav commented Aug 29, 2017 • edited

nicojs commented Aug 29, 2017

nicojs commented Aug 29, 2017

sharikovvladislav commented Aug 29, 2017

nicojs commented Aug 29, 2017

sharikovvladislav commented Aug 29, 2017

nicojs left a comment

Choose a reason for hiding this comment

nicojs commented Aug 30, 2017

sharikovvladislav commented Aug 30, 2017 • edited

sharikovvladislav commented Aug 31, 2017

nicojs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharikovvladislav Sep 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharikovvladislav Sep 1, 2017 • edited

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

sharikovvladislav Sep 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharikovvladislav Sep 1, 2017 • edited

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

sharikovvladislav commented Sep 1, 2017 • edited

sharikovvladislav commented Sep 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharikovvladislav Sep 1, 2017 • edited

Choose a reason for hiding this comment

sharikovvladislav commented Sep 1, 2017

nicojs commented Sep 1, 2017

sharikovvladislav commented Sep 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicojs commented Sep 2, 2017

sharikovvladislav commented Sep 2, 2017 • edited

sharikovvladislav commented Sep 2, 2017

nicojs commented Sep 3, 2017

sharikovvladislav commented Aug 28, 2017 •

edited

sharikovvladislav commented Aug 28, 2017 •

edited

sharikovvladislav commented Aug 29, 2017 •

edited

sharikovvladislav commented Aug 30, 2017 •

edited

sharikovvladislav Sep 1, 2017 •

edited

sharikovvladislav Sep 1, 2017 •

edited

sharikovvladislav Sep 1, 2017 •

edited

sharikovvladislav Sep 1, 2017 •

edited

sharikovvladislav commented Sep 1, 2017 •

edited

sharikovvladislav commented Sep 1, 2017 •

edited

sharikovvladislav Sep 1, 2017 •

edited

sharikovvladislav commented Sep 2, 2017 •

edited