implement spyOnProperty method by phra · Pull Request #5107 · jestjs/jest

phra · 2017-12-18T13:38:04Z

Summary

In order to be able to spy on getters/setters we need to backport spyOnProperty method from Jasmine.

~~implement spyOnProperty method~~
~~implement tests~~
~~merge top level invocation of spyOn/spyOnProperty~~

Test plan

tests are implemented.

SimenB · 2017-12-18T13:48:30Z

-  parent?: Module,
-  paths?: Array<Path>,
-  require?: (id: string) => any,
+    exports: any,


please run eslint to fix all these whitespace changes

i fixed every whitespace by hand.. i missed this file probably or it was overwritten later. i will fix it.

Hmm, you shouldn't have to. Just yarn eslint . --fix should be enough. I don't have issues with the lint integration, but I don't use VS code either

SimenB · 2017-12-18T13:49:16Z

  "jest.pathToJest": "yarn jest --",
-  "prettier.eslintIntegration": true
+  "prettier.eslintIntegration": true,
+  "editor.formatOnSave": false


is this needed?

I prefer not

i can revert the dedicated commit if you prefer that way.

SimenB · 2017-12-18T13:55:02Z

  }

+  spyOnProperty(object: any, propertyName: any, accessType = 'get'): any {
+    if (typeof object !== 'object' && typeof object !== 'function') {


this should more closely match the normal spyOn.

actually this if is the same of the spyOn above. what do you mean? i have adapted the code to with with a property descriptor instead of a normal property. do i have missed something here?

Stuff like "// IE 8 doesn't support definePropery on non-DOM nodes" etc is not needed.

But anyways, can we make jest.spyOn work, so this new function is unnecessary?

keep in mind that making spyOn work with getters/setters will introduce a breaking change between jasmine and jasmine-light shipped with jest, adding a little extra step during migrations to jest.
for this reason, i actually prefer following the original project and create a new method spyOnProperty for that.

SimenB · 2017-12-18T13:55:40Z

Any reason why we cannot make spyOn work on getters?

phra · 2017-12-18T14:04:01Z

@SimenB i was thinking about it too. in the end i just followed the original jasmine PR to be more compliant with the original library.

cpojer · 2017-12-18T14:08:18Z

Making spyOn work itself would be awesome.

phra · 2017-12-18T14:19:21Z

i've reverted the commit for editor.formatOnSave and fixed the indentation.

phra · 2017-12-19T11:13:00Z

yesterday evening i tried to fix every errors in the tests.. one snapshots is still failing.

Summary of all failing tests
 FAIL  integration_tests/__tests__/jest.config.js.test.js (10.23s)
  ● traverses directory tree up until it finds jest.config

    expect(value).toMatchSnapshot()
    
    Received value does not match stored snapshot 2.
    
    - Snapshot
    + Received
    
    - "PASS ../../../__tests__/a-banana.js
    + "PASS ../../../__tests__/a-banana.js (6.798s)
        ✓ banana
        ✓ abc
      
      "

      60 |   const {rest, summary} = extractSummary(stderr);
      61 |   expect(status).toBe(0);
    > 62 |   expect(rest).toMatchSnapshot();
      63 |   expect(summary).toMatchSnapshot();
      64 | });
      65 | 
      
      at Object.<anonymous>.test (integration_tests/__tests__/jest.config.js.test.js:62:16)

what's the next steps for this feature? at the moment i had to disable the unit tests in a firebase project due to the fact that i can't spy on getters with jest but i would like to restore the tests in these days..

SimenB · 2017-12-19T11:48:23Z

#5132 should fix the failure you're seeing

what's the next steps for this feature?

We need tests showing that the new function works

phra · 2017-12-19T12:02:51Z

ok, i will rebase this branch when #5132 is merged. in the meanwhile i will try to implement some tests for this feature.

codecov-io · 2017-12-19T13:04:03Z

Codecov Report

Merging #5107 into master will decrease coverage by 0.11%.
The diff coverage is 48.21%.

@@            Coverage Diff             @@
##           master    #5107      +/-   ##
==========================================
- Coverage    60.7%   60.59%   -0.12%     
==========================================
  Files         201      201              
  Lines        6691     6748      +57     
  Branches        4        3       -1     
==========================================
+ Hits         4062     4089      +27     
- Misses       2628     2658      +30     
  Partials        1        1

Impacted Files	Coverage Δ
...ackages/jest-jasmine2/src/jasmine/jasmine_light.js	`0% <0%> (ø)`	⬆️
packages/jest-jasmine2/src/jasmine/spy_registry.js	`0% <0%> (ø)`	⬆️
packages/jest-mock/src/index.js	`86.18% <100%> (+1.34%)`	⬆️

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 e74fbff...224eb25. Read the comment docs.

phra · 2017-12-25T21:15:28Z

@SimenB i've implemented the tests and CI is green.. can you please review the code changes? thanks.

SimenB · 2017-12-25T23:37:40Z

Why can't we use jest.spyOn? I don't understand why a new method is needed

phra · 2017-12-27T14:12:47Z

in the end i just followed the original jasmine PR to be more compliant with the original library.

@SimenB as i said above, to maintain consistency with jasmine apis.

SimenB · 2017-12-27T17:00:34Z

Parity with Jasmine's APIs is non-goal (we want to drop jasmine entirely, #4362).

phra · 2018-01-04T11:39:48Z

@SimenB are we sure that changing behaviour of spyOn in this way will be retro compatible?

btw a benefit that i see valuable of having the same semantics of jasmine is letting people easily migrate from jasmine to jest without having to refactor the tests, that i consider a big sell point actually.

also, besides the fact that we want to drop jasmine entirely, why do you want to introduce a breaking change that is not strictly (i.e. technically) required?

phra · 2018-01-08T16:17:16Z

@SimenB any feedback? i need this PR shipped to restore unit tests involving angularfire2, that starting from the latest major version it's exposing submodules via getters instead of normal properties.

SimenB · 2018-01-08T16:21:02Z

@thymikee @cpojer I think this makes sense, WDYT?

cpojer · 2018-01-08T16:43:37Z

+  let uncheckedKeys;

  if (uncheckedCount) {
+    uncheckedKeys = snapshotState.getUncheckedKeys();


why was this changed? This seems unrelated.

i got runtime errors when developing this feature when uncheckedCount is falsy. do you think that this change is not required?

I think this should be changed in a separate commit, with a test that is fixed by your change. Please revert it from this PR and send one for this issues specifically. Thank you.

cpojer · 2018-01-08T16:44:15Z

+  this._spyOnProperty = function(obj, propertyName, accessType = 'get') {
+    if (!obj) {
+      throw new Error(
+        'spyOn could not find an object to spy upon for ' + propertyName + '',


There is an empty string at the end. Can we end the sentence with a .? Also, can we prefix the error with "Jest: "?

i will do it.

@cpojer in the end i've used getErrorMsg() function as in the spyOn function. is that better than manual format the string, isn'it?
see bd44ca5

cpojer · 2018-01-08T16:44:20Z

+    }
+
+    if (!propertyName) {
+      throw new Error('No property name supplied');


Same as above

i will fix it.

cpojer · 2018-01-08T16:45:59Z

Can you format all the error messages properly?

What if somebody wants to spy both on the getter and setter of a field, with separate spies?

phra · 2018-01-08T16:50:55Z

@cpojer good point. everything should work simply calling two times the spyOn method with get and set parameters, due to this line: https://github.com/phra/jest/blob/95f0ab9a6f6faa989e1590ba9695af854d30d7c7/packages/jest-jasmine2/src/jasmine/spy_registry.js#L197

what do you think about my reply?

phra · 2018-01-08T17:49:17Z

@cpojer regarding the error messages formatting, in the end i've used getErrorMsg() function as in the spyOn function. is that better than manual format the string, isn'it?
see bd44ca5

phra · 2018-01-15T10:09:06Z

@SimenB @cpojer i don't see this feature reported in the changelog.. should i send a PR for that?

EDIT: btw i've finally restored my failing tests. 😄

SimenB · 2018-01-15T11:44:31Z

@phra yes please!

`spyOn` in getters and setters is available starting from Jest v. 22.0.5, see jestjs#5107

phra · 2018-01-15T12:42:59Z

@SimenB done! #5314

`spyOn` in getters and setters is available starting from Jest v. 22.0.5, see #5107

github-actions · 2021-05-12T23:07:11Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

facebook-github-bot added the CLA Signed ✔️ label Dec 18, 2017

phra force-pushed the feat/spy-on-property branch from ab6e1db to 98acdff Compare December 18, 2017 13:42

SimenB reviewed Dec 18, 2017

View reviewed changes

SimenB requested a review from boujeepossum December 18, 2017 13:48

SimenB reviewed Dec 18, 2017

View reviewed changes

phra mentioned this pull request Dec 18, 2017

Implement spies for get/set functions on accessor properties jasmine/jasmine#943

Closed

phra force-pushed the feat/spy-on-property branch 2 times, most recently from 6c91646 to 9c751da Compare December 18, 2017 14:17

phra force-pushed the feat/spy-on-property branch 2 times, most recently from 38860c7 to e9147a6 Compare December 18, 2017 20:58

phra added 3 commits December 19, 2017 13:53

feat: implement spyOnProperty method, fixes jestjs#5106

1a3b82a

style: fix indentation of mock, runtime, jasmine2

4cfb737

test: fix failing tests for spyOnProperty

65d884d

phra force-pushed the feat/spy-on-property branch from e9147a6 to 65d884d Compare December 19, 2017 12:54

phra added 2 commits December 25, 2017 21:56

test: implement tests for jestjs#1214

a4a6313

style: fix eslint errors

d26337c

phra and others added 6 commits January 6, 2018 00:26

docs: add documentation for accessType argument of spyOn

0a5e083

docs: fix typo in spyOn docs

c0894d9

test(spyOn): fix typo in should throw on invalid input

be83146

test(spyOn): add tests for setters

e5aa9a6

docs(spyOn): add example for spying on setters

26b5ddb

style: fix eslint errors

95f0ab9

SimenB approved these changes Jan 8, 2018

View reviewed changes

cpojer reviewed Jan 8, 2018

View reviewed changes

phra and others added 3 commits January 8, 2018 17:59

refactor: format error messages with getErrorMsg()

bd44ca5

style: fix eslint errors

37093f6

revert: restore snapshotState.getUncheckedKeys()

224eb25

cpojer merged commit 490f435 into jestjs:master Jan 8, 2018

phra deleted the feat/spy-on-property branch January 8, 2018 18:26

phra added a commit to phra/jest that referenced this pull request Jan 15, 2018

chore: include jestjs#5107 in CHANGELOG.md

6dfff65

`spyOn` in getters and setters is available starting from Jest v. 22.0.5, see jestjs#5107

phra mentioned this pull request Jan 15, 2018

chore: include #5107 in CHANGELOG.md #5314

Merged

cpojer pushed a commit that referenced this pull request Jan 15, 2018

chore: include #5107 in CHANGELOG.md (#5314)

158cd63

`spyOn` in getters and setters is available starting from Jest v. 22.0.5, see #5107

renovate-tests mentioned this pull request Feb 6, 2018

Update dependency jest to v22 - autoclosed renovate-tests/changelog-md#3

Closed

renovate Bot mentioned this pull request Feb 14, 2018

fix(deps): update dependency jest to ^22.0.0 manekinekko/devoxx-assistant#3

Open

github-actions Bot locked as resolved and limited conversation to collaborators May 12, 2021

Uh oh!

Conversation

phra commented Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimenB commented Dec 18, 2017

Uh oh!

phra commented Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpojer commented Dec 18, 2017

Uh oh!

phra commented Dec 18, 2017

Uh oh!

phra commented Dec 19, 2017

Uh oh!

SimenB commented Dec 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phra commented Dec 19, 2017

Uh oh!

codecov-io commented Dec 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

phra commented Dec 25, 2017

Uh oh!

SimenB commented Dec 25, 2017

Uh oh!

phra commented Dec 27, 2017

Uh oh!

SimenB commented Dec 27, 2017

Uh oh!

phra commented Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phra commented Jan 8, 2018

Uh oh!

SimenB commented Jan 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phra Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpojer commented Jan 8, 2018

Uh oh!

phra commented Dec 18, 2017 •

edited

Loading

phra commented Dec 18, 2017 •

edited

Loading

SimenB commented Dec 19, 2017 •

edited

Loading

codecov-io commented Dec 19, 2017 •

edited

Loading

phra commented Jan 4, 2018 •

edited

Loading

phra Jan 8, 2018 •

edited

Loading

phra commented Jan 8, 2018 •

edited

Loading

phra commented Jan 15, 2018 •

edited

Loading

SimenB commented Jan 15, 2018 •

edited

Loading