[jasmine] Improve various library types#38412
[jasmine] Improve various library types#38412mrcrane merged 13 commits intoDefinitelyTyped:masterfrom zvirja:improve-jasmine-basic-typings
Conversation
typescript-bot
added
Popular package
|
@zvirja Thank you for submitting this PR! 🔔 @borisyankov @theodorejb @davidparsson @gmoothart @lukas-zech-software @Engineer2B @cyungmann @Roaders @devoto13 @FDIM @pe8ter @kolodny @stephenfarrar @ndunks @sjelin @gkalpak @SeanSobey - please review this PR in the next few days. Be sure to explicitly select If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead. |
|
👋 Hi there! I’ve run some quick performance metrics against master and your PR. This is still an experiment, so don’t panic if I say something crazy! I’m still learning how to interpret these metrics. Let’s review the numbers, shall we? jasmine/v3Comparison details for jasmine/v3 📊
It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place. jasminewd2/v2
Wow, it looks like all the big movers moved in the right direction! Way to go! 🌟 saywhen/v1Comparison details for saywhen/v1 📊
First off, note that the system varied slightly between these two runs, so you’ll have to take these measurements with a grain of salt. It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place. If you have any questions or comments about me, you can ping |
sjelin
left a comment
sjelin
left a comment
There was a problem hiding this comment.
I can only sign off on the jasminewd2 dependency update, since that's the only one I'm a maintainer of. That bit LGTM though.
|
This broke a couple of build targets at Google, I'm still digging in to figure out what and why. I can update when I have some more time |
Improve toBe(), toEqual(), toMatch(), toHaveClass() documentation to avoid confusion around them.
The jasminewd2 library supports jasmine 2 only, therefore point typings to the correct reference.
|
Updated numbers for you here from 87bfbdf: jasmine/v3Comparison details for jasmine/v3 📊
It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place. jasminewd2/v2Comparison details for jasminewd2/v2 📊
Wow, it looks like all the big movers moved in the right direction! Way to go! 🌟 saywhen/v1Comparison details for saywhen/v1 📊
Wow, it looks like all the big movers moved in the right direction! Way to go! 🌟 |
typescript-bot
added
the
Unmerged
|
A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped! |
|
Whoops, sorry, this LGTM, I spot checked this and it broke things that should be broken which this PR caught. Good job and thanks! |
typescript-bot
added
Owner Approved
typescript-bot
removed
the
Unmerged
|
I just published |
|
I just published |
| // TypeScript Version: 2.8 | ||
|
|
||
| /// <reference types="jasmine" /> | ||
| /// <reference types="jasmine/v2" /> |
There was a problem hiding this comment.
Joining the party late, but I don't think this change is correct. According to the docs, jasminewd2/tsconfig.json should have been updated instead, mapping jasmine to jasmine/v2.
There was a problem hiding this comment.
With this change published, I have errors similar to Argument of type 'Whatever' is not assignable to parameter of type 'string'. in the parameter of withArgs().
Also i'm no longer able to use jasmine.ObjectContaining() to match a partial representation of a type in withArgs().
Argument of type 'ObjectContaining<{ property: string; }>' is not assignable to parameter of type 'SomeObject'.
Type 'ObjectContaining<{ property: string; }>' is missing the following properties from type 'SomeObject': otherProperty, blah-blah, and 10 more.
I have yet to come up with a bare reproduction, however any tips are welcome in the meantime.
There was a problem hiding this comment.
I'm very sorry for the mess created around - it's always annoying 😖 I had only good intentions to make jasmine usage better.
Wouldn't a major (or at least minor) version bumb be appropriate (based on this doc)?
If you are speaking about jasmine library, then no. There are no breaking changes in the types itself and the previously compatible code is still compatible. Of course, new types are more stricter, but that's always the case when you improve type coverage.
The reason jasminewd2 got affected is because that project was not configured correctly. They must have depended on jasmine v2 instead of pretending being compatible with v3. See more detail here.
According to the docs, jasminewd2/tsconfig.json should have been updated instead, mapping jasmine to jasmine/v2.
That's for spotting it - I read that article, but missed the part about dependent packages. I'll try to give it a go. Anyway, I think it still could have limitations according to this answer.
@AndrejEPAM
With this change published, I have errors similar to Argument of type 'Whatever' is not assignable to parameter of type 'string'. in the parameter of withArgs().
Indeed now we check types for the parameters. You should pass only compatible types. In your case typescript is not making widening, which has been fixed in later versions of TS. Notice, if you don't care about value, just use jasmine.anything() matcher.
Also i'm no longer able to use jasmine.ObjectContaining() to match a partial representation of a type in withArgs().
Strange, as we have test case for that. I've just tried again and the code below worked as a charm:
const spy = jasmine.createSpyObj<{
method: (arg: { prop1: string, prop2: number }) => void }
>(['method']);
expect(spy.method)
.toHaveBeenCalledWith(jasmine.objectContaining({ prop1: "forty-two" }));If everything is fine with code and it fails, please create a standalone GitHub issue and share scenarios, so they could be investigated in more detail.
Notice, you could also use NonTypedSpyObj<T> instead of SpyObj<T> if you would like to opt-out from type checking. Sometimes it's required if method signature is tricky and typescript cannot deduce type correctly (e.g. you have multiple overloads).
There was a problem hiding this comment.
@zvirja thanks for the valuable tips, will try them and create reproduction, if still applicable. I was using jasmine.objectContaining within withArgs, specifically, it worked like a charm, but it may be unintended/unsupported use of objectContaining.
There was a problem hiding this comment.
@AndrejEPAM Oooh, you mean withArgs. Silly me, overlooked that twice, sorry 😖 See it now. Yes, that's a regression and it should be fixed - I overlooked that you could specify argument matchers there 😟
There was a problem hiding this comment.
@AndrejEPAM JFYI Created a PR to allow arg matchers in withArgs() method: #38738. Thanks for highlighting.
There was a problem hiding this comment.
I have a similar issue with 3.4.2. This code no longer compiles:
expect(ga).toHaveBeenCalledWith("send", jasmine.objectContaining({
hitType: "event",
eventCategory: category,
eventAction: action,
eventLabel: label
}));
|
This pull request breaks our test of all PRs, like this one NG-ZORRO/ng-zorro-antd#4231. Now travis reports: Looking for a fix from your side. 😢 |
|
@wendzhue Would you be so kind to provide me with a Minimal, Complete, and Verifiable example of code that helps to reproduce the scenario? I'm not familiar with your codebase. Also consider creating an issue for easier tracking. |
|
This has broken our codebase, where we use nested matchers in From the code, it looks like matchers are supported for individual arguments, but not for nested properties of arguments, yet Jasmine supports this usage. The workaround is to use |
@wendzhue Looks like your issue is this line: https://github.com/NG-ZORRO/ng-zorro-antd/blob/f0ddb79f1d543aa215a2b25abff6079b43eff9e1/components/date-picker/nz-date-picker.component.spec.ts#L940 (and maybe other similar lines) where the function is declared to take no parameters. Makes it look like the change is now correctly warning you that the function shouldn't be given a parameter. Not sure what you're use-case is so I don't know how to advise, but if you have legitimate reasons for this you could always use |
10 participants
npm test.)npm run lint package-name(ortscif notslint.jsonis present).Motivation
Found a couple of typing gaps in the library and decided to fix those. Also slightly refactored the library typing to simplify project maintenance.
Issues
Closes #30552
Closes #34080
Changes
The following enhancements has been done:
Spy
expect(spy).toHaveBeenCalledWith()createSpyObjspyOnAllFunctionsFunctionMatchersinterfaceMatching
toBe()anymatcher to receive prototypes and symbols onlytoHaveClassto avoid misuseMaintenance / other
jasminenamespace to not pollute global namespaceAnyMethods<>in favor ofNonTypedSpyObj<T>(see motivation here)Anyproperty fromMatchers<T>