Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create integration tests with Jest caching enabled #32

Closed
Igmat opened this issue Oct 17, 2016 · 20 comments
Closed

Create integration tests with Jest caching enabled #32

Igmat opened this issue Oct 17, 2016 · 20 comments
Assignees

Comments

@Igmat
Copy link
Contributor

Igmat commented Oct 17, 2016

This issue is for tracking of implementation of tests for issue #30.
I'm not sure that I'll be able to complete this task soon, but definitely will try to find a way.
Some thoughts:

  1. Create folder like watch-test;
  2. Create helper like runJestInWatch.ts based on runJest.ts with --watch or --watchAll enabled;
  3. This helper have to do next things:
    3a. Run tests in watch-test and collect results*;
    3b. Change test file in watch-test and collect results*;
    3c. Change source file in watch-test and collect results*;
    3d. Change config file in watch-test and collect results*;
    3e. Terminate process;
  4. Create watch.spec.ts that runs this helpers and checks results for correctness.

* I'm not sure that we'll be able to collect correct results before process termination, so this part could be moved to last step of helper.

@Igmat Igmat assigned Igmat and unassigned Igmat Oct 17, 2016
@Igmat
Copy link
Contributor Author

Igmat commented Oct 24, 2016

It is strange that build is failing on semaphore due to compilation error. @kulshekhar, could you check it? Because I've removed @types/es6-shim dependency but it somehow exists on CI server.

But, there is some real problems in this commit.

it('should show the correct error locations in the typescript files without changes', async () => {
    let stderr = await result.getStderrAsync();
    expect(stderr).toContain('Hello.ts:13:11');
    expect(stderr).toContain('Hello.test.ts:9:19');
  });

  it('should show the correct error locations in the typescript files with changes in source file', async () => {
    fs.writeFileSync(path.resolve(__dirname, '../watch-test/Hello.ts'), helloFileUpdate);
    let stderr = await result.getStderrAsync();
    expect(stderr).toContain('Hello.ts:11:11');
    expect(stderr).toContain('Hello.test.ts:9:19');
  });

  it('should show the correct error locations in the typescript files with changes in source file and test file', async () => {
    fs.writeFileSync(path.resolve(__dirname, '../watch-test/__tests__/Hello.test.ts'), testFileUpdate);
    let stderr = await result.getStderrAsync();
    expect(stderr).toContain('Hello.ts:11:11');
    expect(stderr).toContain('Hello.test.ts:11:19');
  });

In this code only first it always works correctly, but second and third sometimes do work and sometimes do not. And as for now I can't find the reason. I guess I'm missing something obvious. Any ideas?

@kulshekhar
Copy link
Owner

kulshekhar commented Oct 24, 2016

@Igmat It seems to be building fine now although there is a failing test.

FAIL tests/tests/watch.spec.ts (11.726s)
● hello_world › should show the correct error locations in the typescript files with changes in source file

  Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.

     at Timeout.e [as _onTimeout] (node_modules/jsdom/lib/jsdom/browser/Window.js:477:19)
     at tryOnTimeout (timers.js:228:11)
     at Timer.listOnTimeout (timers.js:202:5)

● hello_world › should show the correct error locations in the typescript files with changes in source file and test file

   Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.

     at Timeout.e [as _onTimeout] (node_modules/jsdom/lib/jsdom/browser/Window.js:477:19)
     at tryOnTimeout (timers.js:228:11)
     at Timer.listOnTimeout (timers.js:202:5)

This output possibly has the reason why you're getting that error

@kulshekhar
Copy link
Owner

This is weird. I just cloned the repo to a fresh directory, switched to the new branch, ran npm i and npm test and all the tests succeeded

@kulshekhar
Copy link
Owner

And when I log in to the CI machine and do the same thing, it works there as well

@Igmat
Copy link
Contributor Author

Igmat commented Oct 24, 2016

@kulshekhar yeap, I have same problems. On fresh run almost always tests are succeeded, but rerunning them will probably lead to timeout error and I can't find a reason why this is happening.

@kulshekhar
Copy link
Owner

@Igmat This might be an issue related to unresolved promises as stated here https://facebook.github.io/jest/docs/troubleshooting.html#unresolved-promises

Replacing async/await with a promise based implementation seemed to work.
https://semaphoreci.com/k/ts-jest/branches/ci-timeout/builds/10

@kulshekhar kulshekhar mentioned this issue Oct 24, 2016
@kulshekhar
Copy link
Owner

@Igmat turns out this issue was caused due to the nodejs version (I checked against 6.3 and 6.6). It's crazy. I've switched to Travis for CI and that seems to have solved this problem.

@Igmat
Copy link
Contributor Author

Igmat commented Oct 27, 2016

It's really strange bug. While running test first time it works, but after rerun I have this type of error:

UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 2): TypeError: Cannot read property 'addExpectationResult' of undefined

And it doesn't matter what type of async is used (ES6 with yield, ES2017 with --harmony flag, Promise.all, then statement, whatever). Probably something is wrong with child process, but I don't understand what actually...

@kulshekhar
Copy link
Owner

Maybe just switch to using promises for now? It worked when I tested it.

@Igmat
Copy link
Contributor Author

Igmat commented Oct 27, 2016

I can't see your commit for promises implementation, but as I remember you've forgotten to return promise in it function, so that promises were skipped at all, and because all expect statements were inside of them test was passed.

@kulshekhar
Copy link
Owner

It passes with at least V6 & V7
https://travis-ci.org/kulshekhar/ts-jest/builds/171139941

@kulshekhar
Copy link
Owner

kulshekhar commented Oct 27, 2016

Why is the target set to es2017 in tsconfig?

@Igmat
Copy link
Contributor Author

Igmat commented Oct 27, 2016

@kulshekhar, I've tested it with --harmony flag for node v7 (because in this case it supports async/await) and @next TS compiler supports es2017 target, and won't downscale async/await to yield and generators.

@Igmat
Copy link
Contributor Author

Igmat commented Oct 27, 2016

@kulshekhar, I've used your branch and run tests locally 2 times. First it works perfect, second fails with same error as in my branch.
So it doesn't matter what is used, async/await or plain Promise - in both cases it successful at first and failed at second run.

Seems that problem is with spawning and killing child-process, or may be with raising events from it. It could be caused by cross-spawn package. If someone is able to check if test works normally at first and second run with default spawn function from child_process, it would be great (I can't do that on Windows, at least now).

@Igmat
Copy link
Contributor Author

Igmat commented Oct 27, 2016

Ok, I've found a way to test it with default spawn. I just ran two series of tests in row on Travis and build succeeded, of course for node v6 and node v7 only.
It means that problem is really caused by cross-spawn package. So, I'll try to find a workaround.

@satazor
Copy link

satazor commented Oct 27, 2016

@Igmat I think you misunderstood what cross-spawn-with-kill offers.. It adds a kill function to the returned child process instance. Looking at bdd7662 you didn't call .kill(). Am I missing something?

The reasoning for not accepting the idea is that this is a drop-in replacement for spawn, no more no less.

@Igmat
Copy link
Contributor Author

Igmat commented Oct 27, 2016

@satazor, kill() called here. Actually, I don't understand why build fails on Travis, but it worked perfectly on my local machine, what was unreal with cross-spawn.

Ok, I understand the point with drop-in replacement for spawn, but without kill you have incompatible API with default spawn from node.js, see docs. Child processes that returned by spawn always have kill() function.

@satazor
Copy link

satazor commented Oct 27, 2016

@Igmat I've missed that. I understand what you mean. Still https://github.com/kentcdodds/cross-spawn-with-kill is also incompatible with what nodejs offers: it doesn't allow a custom signal :(

@Igmat
Copy link
Contributor Author

Igmat commented Oct 27, 2016

@satazor, and it probably has some issues with running on *nix, cause build weren't failed when I've used default spawn.
But I think it should be implemented in your package, so we can try to make a PR that implements it and use fallback to default kill() implementation on *nix system.
Probably @kentcdodds can help, and I guess we have to continue this discussion in your repo.

@Igmat
Copy link
Contributor Author

Igmat commented Nov 2, 2016

Solved by 42a75c5, so we only have to improve test cases.

@Igmat Igmat closed this as completed Nov 2, 2016
@Igmat Igmat self-assigned this Nov 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants