Import nise #1461

mroderick · 2017-06-17T12:29:35Z

This PR is part of #1453, it replaces fake xhr and fake server with nise

To try it out

Check out this branch
npm install
npm test

mroderick · 2017-06-17T14:02:48Z

Added the MAJOR label because:

944f6cf removes previously deprecated properties
We might break something

It's better that people only get this release through conscious upgrade, and not part of regular install (damn you loose versioning!!!!)

fearphage · 2017-06-28T19:08:04Z

944f6cf removes previously deprecated properties

FYI these changes should end up int he release notes.

coveralls · 2017-07-10T20:30:19Z

Coverage increased (+0.5%) to 95.452% when pulling 2395f35 on mroderick:import-nise into 9365d49 on sinonjs:master.

mroderick · 2017-07-10T20:38:17Z

I have pushed new commits that imports the new nise module from npm.

There are still some deprecations we can remove:

Shall we remove them as part of this MAJOR release, or save them for another day?

coveralls · 2017-07-10T21:10:06Z

Coverage increased (+0.5%) to 95.452% when pulling 69cc06a on mroderick:import-nise into 9365d49 on sinonjs:master.

fatso83 · 2017-07-11T12:52:52Z

Remove, remove, remove!

fatso83 · 2017-07-12T17:30:03Z

I think we can merge this and remove any remaining deprecations in another PR. Anyone opposed?

mroderick · 2017-07-13T08:12:50Z

Anyone opposed?

Me! I am already working on removing the last deprecations, so I'd rather just keep this branch open for a bit longer, than either have master unshippable again or having to maintain another legacy branch.

mroderick · 2017-07-15T06:41:44Z

OK, I think I have mopped up all the previously deprecated stuff.

If we're going to put out a new MAJOR release based on this PR, then I think it deserves a thorough review.

coveralls · 2017-07-15T06:42:27Z

Coverage increased (+0.4%) to 95.381% when pulling 59a87e6 on mroderick:import-nise into 259a330 on sinonjs:master.

mroderick · 2017-07-15T06:44:28Z

I am attending a family event today, so won't be able to contribute anything until tonight at the earliest and perhaps not even until sometime next week.

coveralls · 2017-07-15T06:46:08Z

Coverage increased (+0.4%) to 95.381% when pulling 5eca462 on mroderick:import-nise into 259a330 on sinonjs:master.

mroderick · 2017-07-22T07:35:05Z

Ping @sinonjs/core do you have any further comments for this PR?

fatso83 · 2017-07-22T07:38:59Z

Njet. All good.

fearphage

A few small things like missing tests. But the biggest thign I see is missing documentation about what is being removed.

fearphage · 2017-07-23T03:49:55Z

lib/sinon/collection.js

        if (arguments.length > 2) {
-            return sandboxStub.apply(this, arguments);
+            throw new TypeError("stub(obj, 'meth', fn) has been removed, see documentation");


Remove the duplication. This will still get thrown on line 97 below.

Indeed, well spotted

fearphage · 2017-07-23T03:57:09Z

lib/sinon/stub-non-function-property.js

@@ -3,15 +3,13 @@
 var valueToString = require("./util/core/value-to-string");
 var hasOwnProperty = Object.prototype.hasOwnProperty;

-function stubNonFunctionProperty(object, property, value) {
+function stubNonFunctionProperty(object, property) {


2 things:

This should throw like the other stub method with too many arguments.

The replacement for this usage is not documented (like stub(...).callFake(fn)).

This should throw like the other stub method with too many arguments.

It is internal only, and is called inside sandbox.stub, which does throw for too many arguments.

The replacement for this usage is not documented

The practice of using sandbox.stub for stubbing out properties has been documented for awhile, admittedly, not very well.

I have pushed another commit to improve documentation for stubbing out non-function properties

fearphage · 2017-07-23T04:03:13Z

lib/sinon/stub-non-function-property.js

    var original = object[property];

    if (!hasOwnProperty.call(object, property)) {
        throw new TypeError("Cannot stub non-existent own property " + valueToString(property));
    }

-    object[property] = value;


Looks like most of the property tests were removed. I see negative tests, but no positive tests.

Stubbing properties is the responsibility of collection.js, it has several tests covering this.

fearphage · 2017-07-23T04:04:08Z

lib/sinon/util/core/index.js

@@ -2,7 +2,6 @@

 module.exports = {
    calledInOrder: require("./called-in-order"),
-    configureLogError: require("./log_error"),


If this was accessible, we should document that it was removed even if it was previously deprecated.

fearphage · 2017-07-23T04:04:37Z

lib/sinon.js

@@ -49,20 +41,13 @@ exports.useFakeTimers = fakeTimers.useFakeTimers;
 exports.clock = fakeTimers.clock;
 exports.timers = fakeTimers.timers;

-var event = require("./sinon/util/event");
-exports.Event = deprecated.wrap(event.Event, deprecated.defaultMsg("Event"));


Documentation for all of these changes.

fearphage · 2017-07-23T04:05:27Z

lib/sinon/stub-descriptor.js

-
-// This is deprecated and will be removed in a future version of sinon.
-// We will only consider pull requests that fix serious bugs in the implementation
-function stubDescriptor(object, property, descriptor) {


Documentation.

This was an internal only file.

That sinon.stub only accepts two arguments has been documented in this very commit

fearphage · 2017-07-23T04:06:59Z

test/sandbox-test.js

@@ -597,6 +597,7 @@ describe("sinonSandbox", function () {

        it("allows stubbing setters", function () {
            var object = {
+                foo: undefined,


Did this need to be added?

It is necessary, otherwise we run into the input validation of collection.stub, that checks for own properties on stubbing targets.

TypeError: Cannot stub non-existent own property foo

coveralls · 2017-07-24T07:26:10Z

Coverage increased (+0.4%) to 95.434% when pulling 0570deb on mroderick:import-nise into 259a330 on sinonjs:master.

mroderick · 2017-07-24T07:27:33Z

@fearphage thank you for your thorough review.

I think I have addressed all your comments.

If you're satisfied, then I'll fold the review commit into the appropriate commit and merge it.

fearphage

Are all the event deprecations and removals documented? It looks good for the most part besides my doc concerns.

I also noticed a bum test, but I unblocked the PR.

fearphage · 2017-07-24T13:41:51Z

test/stub-test.js

-            }});
-
-            assert.equals(obj.prop, 43);
+            var stub = createStub(obj, "prop");


This test has no assertions now. Add an assertion or remove the test.

That was a mistake! I've fixed this in the offending commit and have rebased my branch

* sandbox.stub(obj, 'meth', val) * sinon.stub(obj, 'meth', fn) * sinon/util/core

`sinonStub` will take care of validating the length of the arguments

coveralls · 2017-07-26T06:56:29Z

Coverage increased (+0.4%) to 95.434% when pulling 1ef9e71 on mroderick:import-nise into 20eb865 on sinonjs:master.

mroderick · 2017-07-26T07:20:59Z

I think everything has been addressed. Since #1496 is also a MAJOR, I would like to merge that one first and then this one, and then just cut one new MAJOR release, instead of two.

mroderick added the semver:major changes will cause a new major version label Jun 17, 2017

mroderick force-pushed the import-nise branch from 47a16b8 to 62311ad Compare June 22, 2017 18:34

mroderick force-pushed the import-nise branch from 62311ad to 2395f35 Compare July 10, 2017 20:26

mroderick force-pushed the import-nise branch from 2395f35 to 69cc06a Compare July 10, 2017 21:06

mroderick force-pushed the import-nise branch from 3c73348 to 59a87e6 Compare July 15, 2017 06:40

mroderick force-pushed the import-nise branch from 59a87e6 to 5eca462 Compare July 15, 2017 06:44

fearphage requested changes Jul 23, 2017

View reviewed changes

fearphage approved these changes Jul 24, 2017

View reviewed changes

fatso83 mentioned this pull request Jul 25, 2017

Issue 431 #1495

Closed

mroderick added 5 commits July 26, 2017 08:48

Remove hello world test

ca722de

Remove deprecated properties from main module

80946c9

Remove deprecated

74263dd

* sandbox.stub(obj, 'meth', val) * sinon.stub(obj, 'meth', fn) * sinon/util/core

Add nise dependency

ce95b7f

Use fakeXhr, fakeServer and fakeServerWithClock from nise

6c978f8

mroderick added 3 commits July 26, 2017 08:49

Remove obviated files and code caused by nise import

6b1841f

Add documentation for stubbing non-function properties

217b155

Review: remove duplication

1ef9e71

`sinonStub` will take care of validating the length of the arguments

mroderick force-pushed the import-nise branch from 0570deb to 1ef9e71 Compare July 26, 2017 06:51

mroderick merged commit 40f1599 into sinonjs:master Jul 27, 2017

mroderick deleted the import-nise branch July 27, 2017 07:54

mroderick mentioned this pull request Jul 27, 2017

master build failing #1498

Closed

glasserc mentioned this pull request Aug 3, 2017

Update sinon to the latest version 🚀 Kinto/kinto.js#743

Closed

Import nise #1461

Import nise #1461

Conversation

mroderick commented Jun 17, 2017 • edited Loading

To try it out

mroderick commented Jun 17, 2017

fearphage commented Jun 28, 2017

coveralls commented Jul 10, 2017 • edited Loading

mroderick commented Jul 10, 2017

coveralls commented Jul 10, 2017 • edited Loading

fatso83 commented Jul 11, 2017

fatso83 commented Jul 12, 2017

mroderick commented Jul 13, 2017

mroderick commented Jul 15, 2017

coveralls commented Jul 15, 2017

mroderick commented Jul 15, 2017

coveralls commented Jul 15, 2017 • edited Loading

mroderick commented Jul 22, 2017

fatso83 commented Jul 22, 2017

fearphage left a 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

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

coveralls commented Jul 24, 2017 • edited Loading

mroderick commented Jul 24, 2017

fearphage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 26, 2017 • edited Loading

mroderick commented Jul 26, 2017

mroderick commented Jun 17, 2017 •

edited

Loading

coveralls commented Jul 10, 2017 •

edited

Loading

coveralls commented Jul 10, 2017 •

edited

Loading

coveralls commented Jul 15, 2017 •

edited

Loading

coveralls commented Jul 24, 2017 •

edited

Loading

coveralls commented Jul 26, 2017 •

edited

Loading