Skip to content

feat(zone.js): patch jasmine.createSpyObj to make properties enumerable to be true#34624

Closed
JiaLiPassion wants to merge 1 commit intoangular:masterfrom
JiaLiPassion:jasmine-createspyobj
Closed

feat(zone.js): patch jasmine.createSpyObj to make properties enumerable to be true#34624
JiaLiPassion wants to merge 1 commit intoangular:masterfrom
JiaLiPassion:jasmine-createspyobj

Conversation

@JiaLiPassion
Copy link
Copy Markdown
Contributor

@JiaLiPassion JiaLiPassion commented Jan 3, 2020

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #33657

Close #33657

in jasmine 3.5, there is a new feature, user can pass a properties object to jasmine.createSpyObj

const spy = jasmine.createSpyObj('spy', ['method1'], {prop1: 'foo'});
expect(spy.prop1).toEqual('foo');

This case will not work for Angular TestBed, for example,

describe('AppComponent', () => {
  beforeEach(() => {

    //Note the third parameter
    // @ts-ignore
    const someServiceSpy = jasmine.createSpyObj('SomeService', ['someFunction'], ['aProperty']);

    TestBed.configureTestingModule({
      declarations: [
        AppComponent
      ],
      providers: [
        {provide: SomeService, useValue: someServiceSpy},
      ]
    }).compileComponents();

  });

  it('should create the app', () => {
    //spyObj will have someFunction, but will not have aProperty
    let spyObj = TestBed.get(SomeService);
  });

Because jasmine.createSpyObj will create the aProperty with enumerable=false,
and TestBed.configureTestingModule will try to copy all the properties from spyObj to
the injected service instance. And because enumerable is false, so the property (here is aProperty)
will not be copied.

This PR will monkey patch the jasmine.createSpyObj and make sure the new property's
enumerable=true.

@JiaLiPassion JiaLiPassion requested a review from a team January 3, 2020 05:25
@JiaLiPassion JiaLiPassion added the area: zones Issues related to zone.js label Jan 3, 2020
@ngbot ngbot Bot added this to the needsTriage milestone Jan 3, 2020
@JiaLiPassion JiaLiPassion added the area: testing Issues related to Angular testing features, such as TestBed label Jan 3, 2020
@JiaLiPassion JiaLiPassion force-pushed the jasmine-createspyobj branch from cafc24b to 05c8953 Compare April 10, 2020 03:40
@JiaLiPassion JiaLiPassion added target: patch This PR is targeted for the next patch release and removed state: WIP labels Apr 10, 2020
@pullapprove pullapprove Bot requested a review from mhevery April 10, 2020 03:41
@JiaLiPassion JiaLiPassion force-pushed the jasmine-createspyobj branch from 05c8953 to 74ee80b Compare April 10, 2020 03:49
@JiaLiPassion JiaLiPassion changed the title feat: patch jasmine.createSpyObj feat(zone.js): patch jasmine.createSpyObj to make properties enumerable to be true Apr 10, 2020
@JiaLiPassion JiaLiPassion force-pushed the jasmine-createspyobj branch from 74ee80b to bb5bbac Compare May 1, 2020 01:53
@mhevery mhevery added the action: merge The PR is ready for merge by the caretaker label May 6, 2020
@mhevery
Copy link
Copy Markdown
Contributor

mhevery commented May 6, 2020

presubmit

Comment thread packages/zone.js/lib/jasmine/jasmine.ts Outdated
@JiaLiPassion JiaLiPassion force-pushed the jasmine-createspyobj branch from bb5bbac to 30063e3 Compare May 14, 2020 22:21
@AndrewKushnir AndrewKushnir added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels Jun 11, 2020
@AndrewKushnir
Copy link
Copy Markdown
Contributor

FYI, I re-labeled this PR to be "master-only", since it's marked as a feature (so it should not go to the patch branch in this case). Thank you.

@AndrewKushnir AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Jun 11, 2020
@AndrewKushnir
Copy link
Copy Markdown
Contributor

Presubmit

@JiaLiPassion
Copy link
Copy Markdown
Contributor Author

@AndrewKushnir, got it, thank you.

@AndrewKushnir
Copy link
Copy Markdown
Contributor

@JiaLiPassion FYI the presubmit looks good. Since this PR is a feature, it may need to wait until master becomes available for v10.1 features. Thank you.

@JiaLiPassion
Copy link
Copy Markdown
Contributor Author

@AndrewKushnir, got it, thank you.

@AndrewKushnir AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit and removed action: presubmit The PR is in need of a google3 presubmit labels Jun 24, 2020
@AndrewKushnir
Copy link
Copy Markdown
Contributor

Presubmit #2 + Global presubmit

@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Jun 29, 2020
@AndrewKushnir AndrewKushnir added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jun 29, 2020
@AndrewKushnir
Copy link
Copy Markdown
Contributor

FYI, global presubmit went well, but the merge script indicated that this PR needs a rebase. @JiaLiPassion could you please rebase it when you get a chance? Thank you.

…le to be true

Close angular#33657

in jasmine 3.5, there is a new feature, user can pass a properties object to `jasmine.createSpyObj`

```
const spy = jasmine.createSpyObj('spy', ['method1'], {prop1: 'foo'});
expect(spy.prop1).toEqual('foo');
```

This case will not work for Angular TestBed, for example,

```
describe('AppComponent', () => {
  beforeEach(() => {

    //Note the third parameter
    // @ts-ignore
    const someServiceSpy = jasmine.createSpyObj('SomeService', ['someFunction'], ['aProperty']);

    TestBed.configureTestingModule({
      declarations: [
        AppComponent
      ],
      providers: [
        {provide: SomeService, useValue: someServiceSpy},
      ]
    }).compileComponents();

  });

  it('should create the app', () => {
    //spyObj will have someFunction, but will not have aProperty
    let spyObj = TestBed.get(SomeService);
  });
```

Because `jasmine.createSpyObj` will create the `aProperty` with `enumerable=false`,
and `TestBed.configureTestingModule` will try to copy all the properties from spyObj to
the injected service instance. And because `enumerable` is false, so the property (here is aProperty)
will not be copied.

This PR will monkey patch the `jasmine.createSpyObj` and make sure the new property's
`enumerable=true`.
@JiaLiPassion JiaLiPassion force-pushed the jasmine-createspyobj branch from 30063e3 to 53a1c71 Compare June 29, 2020 17:53
@AndrewKushnir AndrewKushnir removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jun 29, 2020
@JiaLiPassion
Copy link
Copy Markdown
Contributor Author

Hi, @AndrewKushnir, thanks and I just rebased the PR, it seems the aio payload size failed, but I think it is not related to this PR, please review.

@AndrewKushnir
Copy link
Copy Markdown
Contributor

Thanks for rebasing this PR @JiaLiPassion 👍 The test_aio was a flake and CI is now "green".

@angular-automatic-lock-bot
Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Jul 30, 2020
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
…le to be true (angular#34624)

Close angular#33657

in jasmine 3.5, there is a new feature, user can pass a properties object to `jasmine.createSpyObj`

```
const spy = jasmine.createSpyObj('spy', ['method1'], {prop1: 'foo'});
expect(spy.prop1).toEqual('foo');
```

This case will not work for Angular TestBed, for example,

```
describe('AppComponent', () => {
  beforeEach(() => {

    //Note the third parameter
    // @ts-ignore
    const someServiceSpy = jasmine.createSpyObj('SomeService', ['someFunction'], ['aProperty']);

    TestBed.configureTestingModule({
      declarations: [
        AppComponent
      ],
      providers: [
        {provide: SomeService, useValue: someServiceSpy},
      ]
    }).compileComponents();

  });

  it('should create the app', () => {
    //spyObj will have someFunction, but will not have aProperty
    let spyObj = TestBed.get(SomeService);
  });
```

Because `jasmine.createSpyObj` will create the `aProperty` with `enumerable=false`,
and `TestBed.configureTestingModule` will try to copy all the properties from spyObj to
the injected service instance. And because `enumerable` is false, so the property (here is aProperty)
will not be copied.

This PR will monkey patch the `jasmine.createSpyObj` and make sure the new property's
`enumerable=true`.

PR Close angular#34624
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: testing Issues related to Angular testing features, such as TestBed area: zones Issues related to zone.js cla: yes target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Jasmine createSpyObj w/ properties: Properties lost when spy is provided to and retrieved from TestBed

5 participants