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

[META] fork / upstream maintenance discussion #3

Open
cspotcode opened this issue Sep 11, 2021 · 45 comments
Open

[META] fork / upstream maintenance discussion #3

cspotcode opened this issue Sep 11, 2021 · 45 comments

Comments

@cspotcode
Copy link
Collaborator

cspotcode commented Sep 11, 2021

In NagRock/issues/212

There is discussion about adding maintainers to the original ts-mockito project.

I still hope the author, NagRock, is able to add maintainers. Barring that, we may want to collaborate on this fork: https://npmjs.com/package/@johanblumenberg/ts-mockito

I also have access to the @TypeStrong org, and we can consider moving the project there.


Other links:

This fork needs passing tests, being discussed in #2

@cspotcode cspotcode pinned this issue Sep 11, 2021
@LironHazan
Copy link
Collaborator

All tests are passing now, the node version on git workflow was updated to v14.15.5
Future improvements IMO:

@LironHazan
Copy link
Collaborator

Should open a PR for merging #194
Fixed problem with a mock resolving a mocked value with Promises by @jlkeesey
Seems there are 2 discussions around it:
#191
#219
The PR was approved but never merged
@cspotcode what is the acceptable approach of porting PR's opened for the original repo into the forked repo?
Not sure it's possible to access the #194 commits from the forked repo, will just copying and leaving a comment with the issue id near the fix be OK?

@cspotcode
Copy link
Collaborator Author

Not sure it's possible to access the NagRock#194 commits from the forked repo, will just copying and leaving a comment with the issue id near the fix be OK?

The pull request creation UI should allow creating a pull request from any branch to any branch. Once I pick the correct forks and branches in the dropdowns, it appears to show me the correct commits.

image

@cspotcode
Copy link
Collaborator Author

I created PR #5 for this.

@LironHazan
Copy link
Collaborator

@cspotcode awesome! thanks I'll try doing that myself with another PR

@LironHazan
Copy link
Collaborator

Ported a feature PR #140 mock free functions

@LironHazan
Copy link
Collaborator

Ported #139 Matcher types

@cspotcode
Copy link
Collaborator Author

I noticed that the johanblumenberg README has a list of features it adds which are not in the upstream library:
https://github.com/johanblumenberg/ts-mockito#johanblumenbergts-mockito

Maybe we can try to port all of those features.

@cspotcode
Copy link
Collaborator Author

I sent a support ticket to npm support requesting a transfer of the @TypeStrong scope. If we want, and if they approve the transfer, we can publish ts-mockito under the @typestrong/ts-mockito scope.

@LironHazan
Copy link
Collaborator

@cspotcode I know I already opened PR's for porting features from @johanblumenberg but come to think of it after reading the latest comments on the maintenance thread I realized that maybe it would be better to resolve un-addressed "pains" - issues first and have a clear agenda of how it would be maintained outside of the original repo in case the author wouldn't intend to merge the fork into the original repo, I understand that most of the participants of the thread would like to see the original repo maintained but I have a lack of faith in personal repos in situation like those and IMO a heavily used project should have a community with a clear code of conduct, agenda, skilled developers willing to contribute - saying this carefully, as I do respect the work that was done by the author and others, I just feel the thread is moving in circles..
If I'll see we're making a progress with @typestrong/ts-mockito I'll start using that..

@cspotcode
Copy link
Collaborator Author

I like moving to TypeStrong. Github will let me transfer this repository to TypeStrong today. Is there more to discuss, or are we ready to do that right now?

@LironHazan
Copy link
Collaborator

@cspotcode Let's rock \m/

@cspotcode
Copy link
Collaborator Author

Transfer complete. You should still have full access. If the permissions got dropped for some reason, let me know and I'll be sure to fix it.

@LironHazan
Copy link
Collaborator

@cspotcode cool I see I have access, I'll try to find the time to replace tslint in eslint after my working hours and maybe go over the issues see if there's some bugs that still happen that can be resolved. LMK when you'll publish to npm, worth adding a badge to the readme.

@cspotcode
Copy link
Collaborator Author

@LironHazan where do you want to publish in the short-term? Ideally, we publish to npm ts-mockito if and when NagRock grants us access. Until then, should we publish to:

  • @cspotcode/ts-mockito
  • @typestrong/ts-mockito
  • @ts-mockito/ts-mockito

I am still waiting to hear from npm support about the @TypeStrong scope. I am also not sure if organizations have granular permissions. We would want to be sure ts-mockito maintainers have access to publish @typestrong/ts-mockito, but not to publish unrelated typestrong packages with different maintainership teams.

I have successfully reserved the @ts-mockito scope.

@LironHazan
Copy link
Collaborator

so if it won't be easy/possible to publish into @typestrong/ts-mockito we can go with @ts-mockito/ts-mockito IMO @cspotcode

@cspotcode
Copy link
Collaborator Author

I did some more googling and finally found what I was looking for.
https://docs.npmjs.com/managing-team-access-to-organization-packages
npm will let us create multiple teams inside an organization, each with access to their own packages. So we can use @typestrong/ts-mockito if/when npm support replies to me. I'll wait a bit longer for them to reply.

@cspotcode
Copy link
Collaborator Author

I did not receive a response from npm for my initial support request; I've sent another. I've included a copy below.

If you would prefer to move ahead publishing to @ts-mockito/ts-mockito, I can invite you to the organization on npm. This will grant you permission to publish.

Type: account or billing issue

Subject: Requesting transfer of parked @TypeStrong scope

We would like to publish @typestrong/ts-mockito from https://github.com/TypeStrong/ts-mockito. I see the @TypeStrong organization is owned by someone else but does not have any published packages. Can we request a transfer of the organization? Is this sufficient information, or do you need anything else? How long does it typically take for a request like this to reach a resolution?

I cannot find contact info for the owner of @TypeStrong, but is there a way I can reach them to discuss a transfer?

I am following the instructions from https://docs.npmjs.com/policies/disputes

Thanks in advance.

@LironHazan
Copy link
Collaborator

We can use @ts-mockito/ts-mockito I don't mind,
bTW I started to gradually configure eslint, the default linter is still tslint but if you'll configure your code-editor to detect the eslint config you'll start getting errors,
running eslint will produce: 422 problems (357 errors, 65 warnings) most of them are of:
no-unsafe-assignment
no-unsafe-return
no-unsafe-call
no-unsafe-member-access
As a result of a massive use of any everywhere LOL, honestly I'm not sure if I'll keep fixing types, won't worth the effort..

@cspotcode
Copy link
Collaborator Author

cspotcode commented Oct 9, 2021 via email

@LironHazan
Copy link
Collaborator

@cspotcode lironhazan thanks

@cspotcode
Copy link
Collaborator Author

How do you want to handle changelogs?

I don't have a strong opinion. For ts-node we use Github Releases, (example) and I've recently started adding issues and pull requests to "milestones" to help me create the release notes.

@LironHazan
Copy link
Collaborator

I don't mind using Github Releases, I've never really managed any changelogs before, I've noticed that some projects uses a CHANGELOG.md but I can't tell what's better

@cspotcode
Copy link
Collaborator Author

Let's use Github Releases since NagRock already does: https://github.com/NagRock/ts-mockito/releases

The big things that I personally try to avoid are:

  • automatically generating changelogs from commit logs. This requires commit logs to be too perfect. Commits can get messy; sometimes multiple PRs implement a single feature. Better to write a changelog by hand after a release is completed, IMO
  • Immutable changelogs. I don't publish the changelog to npm inside the package. If we make a mistake, we should be able to fix it.

Both Github Releases and CHANGELOG.md let us make corrections and amendments to the changelog at any time, which is a good thing.

@cspotcode
Copy link
Collaborator Author

I created #11 which renames from @cspotcode to @ts-mockito so that we can publish to npm.

I also created #12 where we should audit every upstream commit that does not exist in this fork. If there is anything important that we missed, we should find a way to get it merged into this fork.

@LironHazan
Copy link
Collaborator

12 is great I see there're conflicts, I'll review when possible

@cspotcode
Copy link
Collaborator Author

It took a little while, but npm transferred the @TypeStrong scope to me. I'm on vacation this weekend, but I can publish an initial release of @typestrong/ts-mockito when I'm back.

@pauleustice
Copy link

pauleustice commented Jul 15, 2022

@cspotcode Do you have still plan to release this to npm? Or is there another way I can consume it? The original ts-mockito stopped working with Typescript 4.4.2 and I'd like to see if your fork resolves the issue at all.

FYI the issue seems to be that const serviceSpy = spy(someAngularService) (when someAngularService is the service being tested, and not a provided dependency) replaces that service's observables with mock functions (only when they are formed from combineLatest or possibly other RxJS operators). This obviously breaks all the tests.

I created a test repo here

EDIT: Also seems to occur for spying on components.

e.g.

// somecomponent.component.ts

export class SomeComponent {
  someObservable$: Observable<someInterface> = someObservableFromSomewhereElse;
}

// somecomponent.component.spec.ts

describe('SomeComponent', () => {
  beforeEach(() => {
    TestBed.configureTestingModule({
      providers: [],
    }).compileComponents();

    fixture = TestBed.createComponent(SomeComponent);
    component = fixture.componentInstance;

    spyComponent = spy(component);
  })
  
  it('should have an observable', () => {
    console.log(component.someObservable$.toString())
  
    //  Logs out ->
    
    // function () {
    //   var args = [];
    //   for (var _i = 0; _i < arguments.length; _i++) {
    //     args[_i] = arguments[_i];
    //   }
    //   var action = new MethodAction_1.MethodAction(key, args);
    //   _this.methodActions.push(action);
    //   var methodStub = _this.getMethodStub(key, args);
    //   methodStub.execute(args);
    //   return methodStub.getValue();
    // }
  });
}

@LironHazan
Copy link
Collaborator

@pauleustice
Hey I replied on the other thread that our tests which use tsmockito works fine but we're actually not using TestBed, and since we're running with Jest we're also using the jest.spyOn method:

Here's an example of instantiation without the TestBed, the mocked instances are directly injected,

describe(' test query editor', () => {
  let component: QueryEditorComponent;
  const mockedFeaturesFlagService = mock(FeatureFlagsService);
  const mockedS1qlLangService = mock(S1qlLangService);
  const mockedPQLLangService = mock(PowerQueryLangService);
  const mockedQueryEditorService = mock(QueryEditorService);
  const mockedCdr = mock(ChangeDetectorRef);

  beforeEach(() => {
    component = new QueryEditorComponent(
      instance(mockedS1qlLangService),
      instance(mockedPQLLangService),
      instance(mockedQueryEditorService),
      instance(mockedFeaturesFlagService),
      instance(mockedCdr)
    );
  });
  
    it('should emit validation state when invoking updateValidationState', () => {
    jest.spyOn(component.validationState, 'emit');
    component.updateValidationState(true);
    expect(component.validationState.emit).toHaveBeenCalledTimes(1);
    expect(component.validationState.emit).toHaveBeenCalledWith({ isValid: true });
  });

I guess @cspotcode didn't publish to npm yet

@pauleustice
Copy link

pauleustice commented Jul 19, 2022

@LironHazan Thanks for both your replies :) I looked at replacing the ts-mockito spy() with jest's spyOn (we're also using Jest), and while it worked, there were 2 problems:

  • We have thousands of tests, so it's not an option right now
  • It doesn't offer a combination of toHaveBeenCalledTimes and toHaveBeenCalledWith, which you get with ts-mockito
    • This would mean a reduction in our test quality

e.g.

ts-mockito:

verify(serviceSpy.someMethod('foo')).times(3);

jest:

expect(serviceSpy.someMethod). toHaveBeenCalledWith('foo');
expect(serviceSpy.someMethod). toHaveBeenCalledTimes(3);

In Jest, you can't tell that foo was passed three times, just that it was passed and the method was called (with potentially anything) three times. It's a small distinction, but something we use a lot in our test suites.

@LironHazan
Copy link
Collaborator

@pauleustice I see..
As a workaround you can:

  1. Write an eslint rule which will transform the specs to replace the single verify().times with the Jest alternative, but it means loosing the check of calling the fn with specific param x times..
  2. Use patch-package to patch a fix on the original ts-mockito and you'll have a local fix

If you'll have a working local fix you can open a PR to this repo and I'll merge it,
I don't think @cspotcode published @typestrong/ts-mockito to npm yet,
@cspotcode can you publish?
Meanwhile I can upgrade the typescript version

@cspotcode
Copy link
Collaborator Author

Yeah, I can publish this weekend. Is the current state of the code good for publication, and what should the version number be?

@LironHazan
Copy link
Collaborator

@cspotcode I see we're 8 commit behind, I'll try to update this PR --> #12 first,
version 2.6.1?

@LironHazan
Copy link
Collaborator

@cspotcode I updated the branch, now we're ahead of the origin repo, all local tests passed so I merged

@LironHazan
Copy link
Collaborator

Upgraded TS and karma --> #14

@pauleustice
Copy link

Thanks both. Yep, a schematic of some sort to transform the occurrences is probably the next bet. Thanks for the heads up on patch-package, hadn't heard of that before. Unfortunately I'm not familiar enough with ts-mockito yet to understand the problem.

I did create this test repo which demonstrates the problem though. It occurs with TS >4.3.5.

If you're able to publish this fork as a package I will see if it solves the issue :)

@LironHazan
Copy link
Collaborator

@pauleustice Unfortunately the typescript upgrade didn't help, I just tested locally with your repo :/

 FAIL   mockito-test  apps/mockito-test/src/app/example.service.spec.ts
  ExampleService
    spying and mocking on service under test
      ✓ should spy on ExampleService methods (20 ms)
      ✓ should return mocked value (5 ms)
      ✓ should not mock spied service observable - marbles (4 ms)
      ✓ should not mock spied service observable - subscribe (2 ms)
    mocking observables on providers
      first provider
        ✓ should mock out observable - marbles (2 ms)
        ✓ should mock out observable - subscribe (6 ms)
      second provider
        ✓ should mock out observable - marbles (2 ms)
        ✓ should mock out observable - subscribe (2 ms)
    mocking combineLatest observables on service under test
      ✓ should not mock out rxjs combineLatest (2 ms)
      ✕ should mock out providers with combineLatest - subscribe (3 ms)
      ✕ should not mock for local observables in combineLatest - subscribe (2 ms)
      ✕ should not mock for local observables in combineLatest with no pipe (16 ms)

  ● ExampleService › mocking combineLatest observables on service under test › should mock out providers with combineLatest - subscribe

    TypeError: service.combinedProviderObservable$.subscribe is not a function

      159 |       // The providers have mock values set in beforeEach, so the combinedProviderObservable$
      160 |       // should return those values
    > 161 |       service.combinedProviderObservable$.subscribe(value => {
          |                                           ^
      162 |         expect(value).toEqual('mockedProviderOneObservable/mockedProviderTwoObservable');
      163 |         done();
      164 |       });

      at src/app/example.service.spec.ts:161:43
      at _ZoneDelegate.Object.<anonymous>._ZoneDelegate.invoke (../../node_modules/zone.js/bundles/zone-testing-bundle.umd.js:409:30)
      at ProxyZoneSpec.Object.<anonymous>.ProxyZoneSpec.onInvoke (../../node_modules/zone.js/bundles/zone-testing-bundle.umd.js:3830:43)
      at _ZoneDelegate.Object.<anonymous>._ZoneDelegate.invoke (../../node_modules/zone.js/bundles/zone-testing-bundle.umd.js:408:56)
      at Zone.Object.<anonymous>.Zone.run (../../node_modules/zone.js/bundles/zone-testing-bundle.umd.js:169:47)
      at Object.wrappedFunc (../../node_modules/zone.js/bundles/zone-testing-bundle.umd.js:4331:34)

  ● ExampleService › mocking combineLatest observables on service under test › should not mock for local observables in combineLatest - subscribe

    TypeError: service.localCombinedObservable$.subscribe is not a function

      170 |
      171 |       // See 'should not mock spied service observable - subscribe', which passes
    > 172 |       service.localCombinedObservable$.subscribe(value => {
          |                                        ^
      173 |         expect(value).toEqual('realObservableOne/realObservableTwo');
      174 |         done();
      175 |       });

      at src/app/example.service.spec.ts:172:40
      at _ZoneDelegate.Object.<anonymous>._ZoneDelegate.invoke (../../node_modules/zone.js/bundles/zone-testing-bundle.umd.js:409:30)
      at ProxyZoneSpec.Object.<anonymous>.ProxyZoneSpec.onInvoke (../../node_modules/zone.js/bundles/zone-testing-bundle.umd.js:3830:43)
      at _ZoneDelegate.Object.<anonymous>._ZoneDelegate.invoke (../../node_modules/zone.js/bundles/zone-testing-bundle.umd.js:408:56)
      at Zone.Object.<anonymous>.Zone.run (../../node_modules/zone.js/bundles/zone-testing-bundle.umd.js:169:47)
      at Object.wrappedFunc (../../node_modules/zone.js/bundles/zone-testing-bundle.umd.js:4331:34)

  ● ExampleService › mocking combineLatest observables on service under test › should not mock for local observables in combineLatest with no pipe

    TypeError: actual.subscribe is not a function

      180 |       // or the pipe(map())
      181 |
    > 182 |       expect(service.localCombinedObservableNoPipe$).toBeObservable(
          |                                                      ^
      183 |         hot('a', {
      184 |           a: [ 'realObservableOne', 'realObservableTwo' ],
      185 |         }),

      at VirtualAction.<anonymous> (../../node_modules/jasmine-marbles/bundles/jasmine-marbles.umd.js:431:31)
      at VirtualAction.Object.<anonymous>.AsyncAction._execute (../../node_modules/rxjs/src/internal/scheduler/AsyncAction.ts:122:12)
      at VirtualAction.Object.<anonymous>.VirtualAction._execute (../../node_modules/rxjs/src/internal/scheduler/VirtualTimeScheduler.ts:89:28)
      at VirtualAction.Object.<anonymous>.AsyncAction.execute (../../node_modules/rxjs/src/internal/scheduler/AsyncAction.ts:97:24)
      at TestScheduler.Object.<anonymous>.VirtualTimeScheduler.flush (../../node_modules/rxjs/src/internal/scheduler/VirtualTimeScheduler.ts:32:26)
      at TestScheduler.Object.<anonymous>.TestScheduler.flush (../../node_modules/rxjs/src/internal/testing/TestScheduler.ts:155:16)
      at Object.toBeObservableComparer (../../node_modules/jasmine-marbles/bundles/jasmine-marbles.umd.js:453:15)
      at __EXTERNAL_MATCHER_TRAP__ (../../node_modules/expect/build/index.js:386:30)
      at Object.throwingMatcher [as toBeObservable] (../../node_modules/expect/build/index.js:387:15)
      at src/app/example.service.spec.ts:182:54
      at _ZoneDelegate.Object.<anonymous>._ZoneDelegate.invoke (../../node_modules/zone.js/bundles/zone-testing-bundle.umd.js:409:30)
      at ProxyZoneSpec.Object.<anonymous>.ProxyZoneSpec.onInvoke (../../node_modules/zone.js/bundles/zone-testing-bundle.umd.js:3830:43)
      at _ZoneDelegate.Object.<anonymous>._ZoneDelegate.invoke (../../node_modules/zone.js/bundles/zone-testing-bundle.umd.js:408:56)
      at Zone.Object.<anonymous>.Zone.run (../../node_modules/zone.js/bundles/zone-testing-bundle.umd.js:169:47)
      at Object.wrappedFunc (../../node_modules/zone.js/bundles/zone-testing-bundle.umd.js:4331:34)


=============================== Coverage summary ===============================
Statements   : 85.29% ( 29/34 )
Branches     : 75% ( 6/8 )
Functions    : 40% ( 2/5 )
Lines        : 81.48% ( 22/27 )
================================================================================
Test Suites: 1 failed, 1 total
Tests:       3 failed, 9 passed, 12 total
Snapshots:   0 total
Time:        1.776 s
Ran all test suites.

The stacktrace won't imply of any ts-mockito source, can we isolate the Angular related abstractions (zone and jasmine wrappers) and make a simple typescript test with a combineLatest?

@pauleustice
Copy link

@LironHazan Yep, I can try that. I'll have a go now. FYI, I previously added logging into ts-mockito's Spy.js and Mock.js files and noticed a difference in the RealMethods between the two TS versions. I'm not sure if this is relevant, but I can paste it here if you think it might be.

@pauleustice
Copy link

@LironHazan I've just pushed that to master on the test repo; basically removed TestBed, and changed the Angular service to a basic class. The tests still error.

Should I create a separate issue for this, since it seems we've established that there is an issue?

@LironHazan
Copy link
Collaborator

@pauleustice Umm maybe it's better to have a separated issue for continuing this discussion,
I pulled and ran the test and got same cryptic stacktrace,

      at src/app/basic.class.spec.ts:151:43
      at _ZoneDelegate.Object.<anonymous>._ZoneDelegate.invoke (../../node_modules/zone.js/bundles/zone-testing-bundle.umd.js:409:30)
      at ProxyZoneSpec.Object.<anonymous>.ProxyZoneSpec.onInvoke (../../node_modules/zone.js/bundles/zone-testing-bundle.umd.js:3830:43)
      at _ZoneDelegate.Object.<anonymous>._ZoneDelegate.invoke (../../node_modules/zone.js/bundles/zone-testing-bundle.umd.js:408:56)
      at Zone.Object.<anonymous>.Zone.run (../../node_modules/zone.js/bundles/zone-testing-bundle.umd.js:169:47)
      at Object.wrappedFunc (../../node_modules/zone.js/bundles/zone-testing-bundle.umd.js:4331:34)

I'll try to isolate it from nx/angular and run on a plain typescript project - will ping soon

@pauleustice
Copy link

@LironHazan Oops, my bad. I forgot to remove the Injectable() bits from the Provider (juggling a lot of stuff today!)

I'm having a look now (by creating a blank Typescript-only project in the workspace), but currently getting lots of failures...

@LironHazan
Copy link
Collaborator

@pauleustice Now I'm getting the jest runner (jest-circus) trace and still no mention to lead to the ts-mockito source,
I tested in front of the upgraded ts-mockito source (ts version 4.4.7) while the host project ts version was 4.4.3 and it failed, but when I downgraded the version of the hosted project to 4.3.5 like you mentioned - the tests passed,
So we should look for the difference between the transpiled test file javascript of 4.3.x and 4.4.x, WDYT?

@pauleustice
Copy link

@LironHazan You wouldn't expect to see ts-mockito in the stack trace though, would you? (Since it isn't that that's erroring). It's just mocking class fields that it shouldn't (I believe) when running spy() which then cause the errors. However, what makes very little sense to me is why Typescript in the project would affect how ts-mockito behaves.

Do you want to raise a PR against my test repo and I can have a look when I get a chance? I have to leave my computer now unfortunately but will continue later on this evening or tomorrow morning.

@LironHazan
Copy link
Collaborator

@pauleustice I wouldn't expect to see ts-mockito errors but I wanted to be sure that it's not,
umm why do you think it mocks fields that shouldn't?

Thinking out load:
With TS 4.4.x following line fails:
service.combinedProviderObservable$.subscribe((value)

TypeError: service.combinedProviderObservable$.subscribe is not a function

I've printed the service for 4.4.x and got:
'**combinedProviderObservable$**': [Function (anonymous)],

And for 4.3.x we get:

     'combinedProviderObservable$': Observable {
        _isScalar: false,
        source: Observable {
          _isScalar: false,
          source: [Observable],
          operator: [CombineLatestOperator]
        },
        operator: MapOperator { project: [Function (anonymous)], thisArg: undefined }
      }

When removing the use of spy it works in 4.4.x,
In TS 4.4.x: When using spy CombineLatest returns a function ref and without the spy the CombineLatest returns an observable.
Won't happen in TS 4.3.x
Wow I don't have a clue,
I'm unfamiliar as well with the spy related code, but what I can do in this repo is adding the test with rxjs that would fail and then we can debug on ts-mockito source.

I'll open a dedicated issue

@cspotcode
Copy link
Collaborator Author

Published to npm, as @typestrong/ts-mockito v2.6.2, because upstream ts-mockito had already published a 2.6.1, so that git tag was already taken.

https://www.npmjs.com/package/@typestrong/ts-mockito

@LironHazan has also been given access to publish new versions.

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