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

Jest jasmine/ts migration #7970

Merged
merged 36 commits into from Mar 5, 2019
Merged

Conversation

@doniyor2109
Copy link
Contributor

doniyor2109 commented Feb 24, 2019

Summary

Migration to TS

Test plan

doniyor2109 and others added 4 commits Feb 24, 2019
@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Feb 24, 2019

Heh, 295 errors 😅 Could you ping me when this is getting more ready for review (or if you have any questions)?

@doniyor2109

This comment has been minimized.

Copy link
Contributor Author

doniyor2109 commented Feb 24, 2019

@SimenB Haha. It is not ready yet. I just wanted to try Draf PR 😄 . I will ping when it is ready

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Feb 24, 2019

BTW, I wouldn't worry too much about typing src/jasmine - it's a fork from jasmine itself and at some point we'll remove it in favor of jest-circus anyways. If it's not too painful, of course go for it 🙂

doniyor2109 and others added 10 commits Mar 1, 2019
…tion

# Conflicts:
#	packages/jest-jasmine2/src/jasmine/Env.js
@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Mar 3, 2019

@doniyor2109 since you commited TS migration part (final), is this ready for review? 🙂 I merged in master since I tweaked a few types here and there in #7998

@doniyor2109

This comment has been minimized.

Copy link
Contributor Author

doniyor2109 commented Mar 3, 2019

@SimenB Yea, it took too long for this migration. I just finished types. However tests are failing because of wired error. Somehow babel does not transpile static fields and property fields correctly. 🤔

@SimenB

This comment was marked as resolved.

Copy link
Collaborator

SimenB commented Mar 3, 2019

Yeah, that's blocked by #7846. You can assign to the class after.

Like this:

static EVAL_RESULT_VARIABLE: 'Object.<anonymous>';

// TODO: Can this be added to the static property?
ScriptTransformer.EVAL_RESULT_VARIABLE = 'Object.<anonymous>';

(the first is only for types, the second is the actual assignment)

doniyor2109 added 3 commits Mar 3, 2019
…tion

# Conflicts:
#	packages/jest-diff/src/index.ts
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 3, 2019

Codecov Report

Merging #7970 into master will decrease coverage by 0.16%.
The diff coverage is 3.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7970      +/-   ##
==========================================
- Coverage   63.31%   63.15%   -0.17%     
==========================================
  Files         263      263              
  Lines       10266    10258       -8     
  Branches     2098     2287     +189     
==========================================
- Hits         6500     6478      -22     
- Misses       3273     3276       +3     
- Partials      493      504      +11
Impacted Files Coverage Δ
...ages/jest-jasmine2/src/expectationResultFactory.ts 88% <ø> (ø)
packages/jest-jasmine2/src/each.ts 0% <ø> (ø)
...ackages/jest-util/src/convertDescriptorToString.ts 0% <ø> (ø) ⬆️
packages/jest-jasmine2/src/pTimeout.ts 100% <ø> (ø)
packages/jest-jasmine2/src/isError.ts 0% <ø> (ø)
packages/jest-snapshot/src/index.ts 32.09% <ø> (ø) ⬆️
packages/jest-jasmine2/src/PCancelable.js 32.25% <ø> (ø) ⬆️
packages/jest-jasmine2/src/treeProcessor.ts 0% <0%> (ø)
...ages/jest-jasmine2/src/jasmine/ReportDispatcher.ts 0% <0%> (ø)
packages/jest-jasmine2/src/jasmine/jasmineLight.ts 0% <0%> (ø)
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c25942...0625619. Read the comment docs.

SimenB and others added 4 commits Mar 3, 2019
…jest-jasmine/ts_migration

# Conflicts:
#	packages/jest-jasmine2/src/index.ts
}
if (currentDeclarationSuite.markedTodo) {
// @ts-ignore TODO Possible error: Suite does not have todo method
suite.todo();

This comment has been minimized.

Copy link
@doniyor2109

doniyor2109 Mar 3, 2019

Author Contributor

todo does not exists on Suite. It might be error. I just ignored for now

if (error instanceof AssertionError) {
checkIsError = false;
// @ts-ignore TODO Possible error: j$.Spec does not have expand property
message = assertionErrorMessage(error, {expand: j$.Spec.expand});

This comment has been minimized.

Copy link
@doniyor2109

doniyor2109 Mar 3, 2019

Author Contributor

This is also might be error.

This comment has been minimized.

Copy link
@doniyor2109

doniyor2109 Mar 3, 2019

Author Contributor

However tests are passing

This comment has been minimized.

Copy link
@SimenB

SimenB Mar 3, 2019

Collaborator

Since the tests are passing, I wouldn't worry too much about it

@SimenB SimenB requested a review from thymikee Mar 3, 2019
@@ -6,8 +6,6 @@
*
*/

'use strict';

describe('iterators', () => {

This comment has been minimized.

Copy link
@SimenB

SimenB Mar 3, 2019

Collaborator

These tests looks like they belong in expect, don't they?

/cc @pedrottimark

This comment has been minimized.

Copy link
@pedrottimark

pedrottimark Mar 3, 2019

Collaborator

Yes, I will make sure that expect covers these test cases.

@@ -6,8 +6,6 @@
*
*/

'use strict';

describe('matchers', () => {

This comment has been minimized.

Copy link
@SimenB

SimenB Mar 3, 2019

Collaborator

same here - seems like they should be in expect

This comment has been minimized.

Copy link
@pedrottimark

pedrottimark Mar 3, 2019

Collaborator

This one might be needed?

describe('matchers', () => {
  it('proxies matchers to expect', () => {
    expect(() => expect(1).toBe(2)).toThrowErrorMatchingSnapshot();
  });
});

This comment has been minimized.

Copy link
@SimenB

SimenB Mar 3, 2019

Collaborator

Yeah, but it doesn't test anything jasmine? I don't understand what it means by "proxies to expect" - it uses expect directly

This comment has been minimized.

Copy link
@pedrottimark

pedrottimark Mar 3, 2019

Collaborator

In other words, Jest test suite is integration test that expect works in jasmine.

This comment has been minimized.

Copy link
@SimenB

SimenB Mar 3, 2019

Collaborator

Right - if expect didn't work in jasmine, the whole test suite would fail. The advantage of using Jest to test Jest 🙂

@SimenB
SimenB approved these changes Mar 3, 2019
SimenB added 2 commits Mar 3, 2019
doniyor2109 and others added 5 commits Mar 4, 2019
…jest-jasmine/ts_migration
…tion
@rubennorte

This comment has been minimized.

Copy link
Contributor

rubennorte commented Mar 5, 2019

This is huge and I just took a quick look. I'll trust @SimenB here. We can merge it, release an alpha and test it in FB code. If everything is good we can just release a new minor after the full TS migration.

Please take into account that jest-circus will be the default in the next major, so Jasmine won't be that critical at that point.

@SimenB SimenB merged commit 7a4ee21 into facebook:master Mar 5, 2019
12 checks passed
12 checks passed
ci/circleci: lint-and-typecheck Your tests passed on CircleCI!
Details
ci/circleci: test-browser Your tests passed on CircleCI!
Details
ci/circleci: test-jest-circus Your tests passed on CircleCI!
Details
ci/circleci: test-node-10 Your tests passed on CircleCI!
Details
ci/circleci: test-node-11 Your tests passed on CircleCI!
Details
ci/circleci: test-node-6 Your tests passed on CircleCI!
Details
ci/circleci: test-node-8 Your tests passed on CircleCI!
Details
ci/circleci: test-or-deploy-website Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
facebook.jest #20190305.4 succeeded
Details
@rubennorte

This comment has been minimized.

Copy link
Contributor

rubennorte commented Mar 5, 2019

Thanks for the huge contribution @doniyor2109!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.