Mobx: Convert tests from JS to TS

[ynejati]

Welcome to MobX. Please provide as much relevant information as possible!

I have a:

1. [x] Idea:
   
   
   
   • [ ] What problem would it solve for you?
   
   
   • [ ] Do you think others will benefit from this change as well and it should in core package (see also mobx-utils)?
   
   
   • [x] Are you willing to (attempt) a PR yourself?
   
   

Please tick the appropriate boxes. Feel free to remove the _other_ sections.

Please be sure to close your issues promptly.

Wanted to bring this up since tests for both mobx-react and mobx-react-lite are now using TypeScript.

Obviously, the conversion here would be a much more involved and lengthy process. But, I'm willing to go the distance if given the green light. When mobx-react was converted, a couple of issues were caught regarding MobX decorators (albeit my fault) and API usage. This will no-doubt result in making use of the any type throughout the test codebase.

Let me know what you think.

[FredyC]

I am not so sure it's a good idea here. Most of the tests are copy-pasta for v4 & v5. Doing any larger refactor is kinda double work. Obviously, we cannot split tests because we wouldn't be able to run them for each version (or I don't know how).

What I had in mind over the time is to actually try to "merge" codebases for both source versions. Or to be precise to extract common parts to be used across those versions. But that's really far fetched.

Anyway, if you really don't mind doing double work, then I guess we are not going to stop you. Perhaps it would be best to split into multiple PRs and convert in small portions, not a whole thing at once which is really hard to review.

[mweststrate]

Also note that a lot of the tests is verifying internal state, doing meta
programming, or testing wrong invocations. So at any rate intentionally
roughly 50% of the test code isn't strongly typeable I'd expect, although
this will probably vary a lot per file (some only test public api's). So
I'd definitely go for a file by file process.

On Mon, Feb 10, 2020 at 2:39 PM Daniel K. [e-mail] wrote:

I am not so sure it's a good idea here. Most of the tests are copy-pasta
for version (v4 & v5). Doing any larger refactor is kinda double work.

Obviously, we cannot split tests because we wouldn't be able to run them
for each version (or I don't know how).

What I had in mind over the time is to actually try to "merge" codebases
for both source versions. Or to be precise to extract common parts to be
used across those versions. But that's really far fetched.

Anyway, if you really don't mind doing double work, then I guess we are
not going to stop you. Perhaps it would be best to split into multiple PRs
and convert in small portions, not a whole thing at once which is really
hard to review.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/mobxjs/mobx/issues/2276?email_source=notifications&email_token=AAN4NBBGNV3JD56KKVRJOIDRCFRLNA5CNFSM4KSN5KVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELIXVWI#issuecomment-584153817,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAN4NBDYBHVQYQGDKG7GJPDRCFRLNANCNFSM4KSN5KVA
.

[ynejati]

Alright, I'm okay with that. I don't mind doing double work (well, in this context). I'll do one file at a time and see how that goes. Thanks!

[FredyC]

Also if you can come up with some way to run a single test suite against each version, it would be most appreciated. As long as those tests are 100% the same, we should try to consolidate them to avoid changes done in a single version only.

From the top of my head dynamic use of require could work, but not sure.

[ynejati]

Haha, for sure. I will see what I can do, within reason. Hopefully we can reduce some of the duplicate logic.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been automatically unmarked as stale. Please disregard previous warnings.

[gilamran]

@ynejati Can you please update on your progress?
If you want, I'm willing to help. This will also help me learn MobX.

[FredyC]

@gilamran As we suggested before, it's definitely not advisable to convert all tests at once. If you have some free time and will, just pick some small portion, announce it here and go ahead.

[gilamran]

Yea got it, will do just that.
Just wanted to see if @ynejati already started, I don't want to duplicate his work.

[gilamran]

@FredyC
I've created a simple PR (#2320) with conversion of the 4 smallest files.
Please review, and I'll continue with the others.

Gil

[ynejati]

Sorry, I've been MIA this last month. Please, feel free to work on this. I'm more than happy to hand this off to you.

[gilamran]

@FredyC While converting the array.js to typescript, I found 2 unknown functions...
maybe deprecated functions?
set and get on an IObservableArray

https://github.com/mobxjs/mobx/blob/master/test/v4/base/array.js#L81

This these are not supported anymore, maybe we should remove them from the tests?

[FredyC]

@gilamran Hm, I am not sure. Looking in the changelog I don't see any mention of such removal.

@urugator can you please confirm?

[gilamran]

I didn't see these functions in the documentation of ObservableArray...

[urugator]

Do NOT remove, these are needed, just the documentation is lacking.
https://github.com/mobxjs/mobx/blob/7bbefff605b2e5298015d6b3460abb7001e45111/src/v4/types/observablearray.ts#L539-L589

[gilamran]

Ok, will leave it as is.
Another issue is when using computed in the options we can pass compareStructural: https://github.com/mobxjs/mobx/blob/master/test/v4/base/makereactive.js#L416

But this is not part of the IComputedValueOptions interface: https://github.com/mobxjs/mobx/blob/master/src/v4/core/computedvalue.ts#L41

Any reason for that?

[gilamran]

Another types issue:
A computed used to have toJSON function that was changed to toJS, both do not exist on the interface...

[gilamran]

Another test issue:
In makereactive.js the test ES5 non reactive props expect this code to throw.
It actually does the throw, but only because te2 is a const that was defined after this code was executed, and it is undefined...

There's a comment above that doesn't sound related to this following line.
Can someone help me understand what's going on there?

Thanks

[mweststrate]

Thanks for doing this! I'll try to look into them later tonight. Feel free
to leave all questions here, or mark those lines with // @ts-ignore TODO to
unblock yourself

On Tue, Mar 31, 2020 at 4:30 PM Gil Amran [e-mail] wrote:

Another test issue:
In makereactive.js the test ES5 non reactive props expect this
https://github.com/mobxjs/mobx/blob/master/test/v4/base/makereactive.js#L453
code to throw.
It actually does the throw, but only because te2 is a const that was
defined after this code was executed, and it is undefined...

There's a comment above that doesn't sound related to this following line.
Can someone help me understand what's going on there?

Thanks

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/mobxjs/mobx/issues/2276#issuecomment-606699753, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/AAN4NBEJ24GYQKDVLVVI5G3RKIEAXANCNFSM4KSN5KVA
.

[FredyC]

@gilamran I suppose you got preoccupied following with this. It doesn't matter, we should definitely postpone any works on this until V6 is out. Alternatively, if you want, do a PR against mobx6.

[mweststrate]

Closing the issue for now as it doesn't have any value. PR's against the v6 branch are welcome if anybody feels for it