implement spyOnProperty method #5107

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

packages/jest-runtime/src/index.js

-  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

.vscode/settings.json

@@ -9,5 +9,6 @@
  "flow.useNPMPackagedFlow": true,
  "javascript.validate.enable": false,
  "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

packages/jest-mock/src/index.js

@@ -691,6 +691,65 @@ class ModuleMockerClass {
    return object[methodName];
  }

+  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

packages/jest-jasmine2/src/index.js


  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

packages/jest-jasmine2/src/jasmine/spy_registry.js

+  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

packages/jest-jasmine2/src/jasmine/spy_registry.js

+    }
+
+    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 aaronabramov 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


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

implement spyOnProperty method #5107

implement spyOnProperty method #5107

Conversation

phra commented Dec 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB commented Dec 18, 2017

phra commented Dec 18, 2017 • edited

cpojer commented Dec 18, 2017

phra commented Dec 18, 2017

phra commented Dec 19, 2017

SimenB commented Dec 19, 2017 • edited

phra commented Dec 19, 2017

codecov-io commented Dec 19, 2017 • edited

Codecov Report

phra commented Dec 25, 2017

SimenB commented Dec 25, 2017

phra commented Dec 27, 2017

SimenB commented Dec 27, 2017

phra commented Jan 4, 2018 • edited

phra commented Jan 8, 2018

SimenB commented Jan 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phra Jan 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer commented Jan 8, 2018

phra commented Jan 8, 2018

phra commented Jan 8, 2018 • edited

phra commented Jan 15, 2018 • edited

SimenB commented Jan 15, 2018 • edited

phra commented Jan 15, 2018

github-actions bot commented May 12, 2021

phra commented Dec 18, 2017 •

edited

phra commented Dec 18, 2017 •

edited

SimenB commented Dec 19, 2017 •

edited

codecov-io commented Dec 19, 2017 •

edited

phra commented Jan 4, 2018 •

edited

phra Jan 8, 2018 •

edited

phra commented Jan 8, 2018 •

edited

phra commented Jan 15, 2018 •

edited

SimenB commented Jan 15, 2018 •

edited