Fix: check configurable on a prop before creating (fixes #1456)

[gyandeeps]

Purpose (TL;DR) - mandatory

Fixes #1456 by making sure propert is made configurable based on the object props.

@lucasfcosta have created a fix with my beast understanding of your proposal. Feedback is welcome. Thanks

[coveralls]

Coverage increased (+0.009%) to 94.972% when pulling ae274c3 on gyandeeps:master into e7dbfd7 on sinonjs:master.

[fatso83]

Thanks for the fix. There is just one thing missing IMHO and that is that, based on its description, the test you are adding does not seem to have anything to do with the code you are adding.

I would move the existing test to the issues test file with a description that references the original issue (see the examples there). I would then add a test where the previous one was with code that does not reference the DOM, but rather creates an object with a property descriptor that would make Sinon fail previously, but that would pass now.

[gyandeeps]

@fatso83 Not sure what you mean but if you remove my source code changes and then run the test I added. It will fail. So i am not entirely sure what you mean by you are adding does not seem to have anything to do with the code you are adding

[fatso83]

It's a semantic issue. The code deals with "check configurable on a prop before creating", but the test is if "it can stub window.innerHeight". There is no direct correlation between the two. Your fix is not directly related to the window property at all - that is just a biproduct of the missing check. (And, as far as I can see, it is not specific to sandboxes either - doesn't it affect non-sandboxed stubs as well?) Therefore, the test shouldn't really have to do with window. You should create an environment the test would fail in before the fix - regardless of you are running in a browser or on the server.

[lucasfcosta]

The core code itself LGTM, but as @fatso83 said, I think it would be good to change these tests so that they would run on every environment, since we're not targeting the window properties themselves, we're testing non-configurable properties and the innerHeight property just happens to be one.

In order to create a non configurable property you could just do:

const obj = {};
Object.defineProperty(obj, 'testProp', {
  configurable: false
});

Then, instead of operating on the window object you should operate on obj.

By doing this you don't need this check anymore too, since you already have a "universally valid" object to test against.

[gyandeeps]

Test code has been fixed. Let me know what you guys think.

[coveralls]

Coverage increased (+0.009%) to 94.972% when pulling 2c88a7e on gyandeeps:master into e7dbfd7 on sinonjs:master.

[fatso83]

Great stuff. Btw, I extracted the test and tested it against master to verify that it broke the previous code.

No objections other than that you could have squashed the fix/amended the previous, but since we can do this directly nowadays I squashed and merged it directly 😄 Thanks!

[mroderick]

This has become v2.3.5

[fatso83]

That was quick.

[mroderick]

That was quick.

Too quick. The build fails in Safari 9.0 😨

[fatso83]

Too bad. The 2000th commit should have been a celebration of our great automation setup 🥇 But 💩 happens 😄 At least we know about it!

@gyandeeps If you wonder went wrong you can check out the logs from the Travis run:

  1) issues #1456 stub window innerHeight:
     Attempting to change access mechanism for an unconfigurable property.
  defineProperty@[native code]
  value@connect.html?url=ws%3A%2F%2Flocalhost%3A1034:1136:30
  connect.html?url=ws%3A%2F%2Flocalhost%3A1034:485:17
  connect.html?url=ws%3A%2F%2Flocalhost%3A1034:478:51
  connect.html?url=ws%3A%2F%2Flocalhost%3A1034:30238:58
  callFn@connect.html?url=ws%3A%2F%2Flocalhost%3A1034:49226:25
  run@connect.html?url=ws%3A%2F%2Flocalhost%3A1034:49218:13
  runTest@connect.html?url=ws%3A%2F%2Flocalhost%3A1034:49714:13
  connect.html?url=ws%3A%2F%2Flocalhost%3A1034:49820:19
  next@connect.html?url=ws%3A%2F%2Flocalhost%3A1034:49632:16
  connect.html?url=ws%3A%2F%2Flocalhost%3A1034:49642:11
  next@connect.html?url=ws%3A%2F%2Flocalhost%3A1034:49566:16
  connect.html?url=ws%3A%2F%2Flocalhost%3A1034:49605:11
  done@connect.html?url=ws%3A%2F%2Flocalhost%3A1034:49173:7
  callFn@connect.html?url=ws%3A%2F%2Flocalhost%3A1034:49244:11
  run@connect.html?url=ws%3A%2F%2Flocalhost%3A1034:49218:13
  next@connect.html?url=ws%3A%2F%2Flocalhost%3A1034:49580:13
  connect.html?url=ws%3A%2F%2Flocalhost%3A1034:49610:9
  timeslice@connect.html?url=ws%3A%2F%2Flocalhost%3A1034:44848:27
Error: Build failed: 1

[gyandeeps]

woops... sorry about this guys... I didn't test it on safari as I use windows... but regardless I wouldn't have thought of testing this on safari. Not sure what the right solution is here because this is very safari thing.