Add v2.0 Migration Guide #1090

jonnyreeves · 2016-07-10T18:06:02Z

Add deprecation guide.
Mark core utils as deprecated.
Mark sinon.Event and friends as deprecated.
Call out deprecation of legacy browsers

Went through all the pull requests opened since the initial kick off of the CJSify work and tried to reason about the changes to the public API.

Wasn't 100% where to the put the migration guide, but the docs folder felt like a good place :)

mroderick · 2016-07-10T20:26:56Z

Very nice! 👍

fatso83 · 2016-07-11T09:30:15Z

Very nice, but we were talking of removing mock as well, right? None of us had any interest in maintaining it, nor knew it well and thought it could be removed in a version 3, so deprecating for v2 could be a start.

P.S. Regarding the deprecated methods should we try to wrap the methods in some kind of deprecation closure that will output a warning? All internal use should by now be removed so that leaves external uses. I was thinking of something like

function deprecated(func, name) {
    var callCount = 0;
    var warn = typeof console !== "undefined" && (console.warn || console.log).bind(console);
    return function() {
        if(callCount++ === 0){
            warn(util.format("[deprecated] The use of sinon.%s has been deprecated. Refer to the migration docs", name));
        }
        func.apply(this, arguments);
    }
}

exports.mock = deprecated( require("./sinon/mock"), "mock") );
// etc

jonnyreeves · 2016-07-11T11:12:58Z

...removing mock as well, right?

If we have concensus on this I'm happy to add it to the notes above.

...wrap the methods in some kind of deprecation closure...

Excellent suggestion @fatso83; I'll update #966 to ensure we deprecate those methods as part of v2

mantoni · 2016-07-11T12:48:33Z

We can use a node util function instead of writing our own: https://nodejs.org/api/util.html#util_util_deprecate_function_string

fatso83 · 2016-07-12T14:55:36Z

@jonnyreeves : not sure which response you are awaiting :)? We have talked several times about removing mock so just add it to the list and merge/squash this in.

jonnyreeves · 2016-07-13T20:07:38Z

All methods called out in the migration guide as deprecated now write a message to the console when they are invoked via the sinon API with no message logged when sinon uses them internally.

I have chosen to not mark sinon.mockas deprecated in this pull as I don't feel we should deprecate methods until we have a migration plan in place; which makes me wonder if we should revisit the decision to remove sinon.test and sinon.testCase from the 2.0 API...

Please let me know your thoughts @fatso83, @mantoni

jonnyreeves · 2016-07-13T20:28:31Z

So in looking at the failing tests I can see my approach for wrapping the Event constructors is flawed as they are going to be invoked via new and therefore can't be applied against null.

What's strange is that the sandbox tests are now failing; I'll have to take a deeper look into that as marking the utils as deprecated should not have had any other side-effects 😖

fatso83 · 2016-07-14T17:06:47Z

@jonnyreeves : What do you mean by migration plan? An actual document in /docs or just some informal sketch of a plan? For test and testCase they can just start using the extracted NPM package. It is perhaps inconsistent not to have deprecated those methods as well, but now have been away for a while, so ...

Re the sandbox thing, I suspect this is similar to Event (not looked into it, though): you might have interfered with/wrapped some prototype ...

jonnyreeves · 2016-07-27T21:12:01Z

@fatso83 I mean that we probably shouldn't mark mock as deprecated in 2.0 unless we are willing to extract it into its own npm module before 2.0 get published otherwise users are not going to have any way to 'un-deprecate' themselves other than removal all mocks from their codebase (which feels a tad extreme :)

IMHO we should ship 2.0 with mock, extract it to a new repo at a later date and then mark mock as deprecated (with a migration plan) for 2.1+ - YMMV.

jonnyreeves · 2016-07-27T21:51:10Z

OK tracked down the failure here, 3 parts:

/src/sinon/util/fake-server.js imports ./core/index.js directly and is expecting that module's exports object to have been mutated by src/sinon.js (in particular, useFakeTimers. This is just plain ugly, but was an easy fix - just had to repoint it to import ../../sinon.js which is what it actually wanted ;)
The sandbox tests still crap out, but the good news is that cherry picking Feature/cjsify sinon sandbox tests #1088 onto this branch magically fixes that issue(!) So I'll push a change up once that has landed.
Emitting a console.warn causes the phantomJS tests to fail... I don't know why... could be an issue in mochify.js? For now I have just downgraded our deprecation message to a console.info which works fine 😁

Just need to patch up deprecating sinon.Event and friends and getting closure on how to deal with sinon.mock...

mantoni · 2016-07-27T22:13:19Z

@jonnyreeves Yes, I think mochify will fail the test if there is a console.warn or console.error call. This is actually somewhere downstream in phantomic to figure out what the exit code is. I should figure out a way to ignore this when using a process.on('exit', ...) listener, like the one installed by mocaccino. 💡

fatso83 · 2016-07-27T22:30:41Z

@jonnyreeves : the migration plan you proposed for mock seems fine, I forgot for a minute (month, actually) we actually have minor releases as well :-)

Re the wrap function: why did you end up not using util.deprecate()?

mantoni · 2016-07-27T22:32:25Z

@jonnyreeves I think there needs to be an additional option for phantomic to not exit with 1 if an error was logged (it is caused here).

fatso83 · 2016-07-27T22:32:55Z

lib/sinon/util/core/deprecated.js

+    return function () {
+        // Watch out for IE7 and below! :(
+        if (typeof console !== "undefined") {
+            if (console.warn) {


Not seeing much trace of the console.info instead of console.warn thing you talked about for PhantomJS?

Ah sorry I have not pushed the changes mentioned above as I was waiting on
the sandbox cjs work to land first.

On Wed, 27 Jul 2016, 23:33 Carl-Erik Kopseng, [email protected]
wrote:

In lib/sinon/util/core/deprecated.js
#1090 (comment):

@@ -0,0 +1,24 @@
+/*eslint no-console: 0 */
+"use strict";
+
+// wrap returns a function that will invoke the supplied function and print a deprecation warning to the console each
+// time it is called.
+exports.wrap = function (func, msg) {

return function () {

// Watch out for IE7 and below! :(

if (typeof console !== "undefined") {

if (console.warn) {

Not seeing much trace of the console.info instead of console.warn thing
you talked about for PhantomJS?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/sinonjs/sinon/pull/1090/files/131555150b9b8785bea6923702d7102e3fe9732f#r72536795,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAMN-edIe5DVMCZV_IBMnUSuJjIdGk3Qks5qZ9yYgaJpZM4JI4nR
.

fatso83 · 2016-07-28T09:58:16Z

This needs to be rebased to include #1088, no?

jonnyreeves · 2016-07-28T10:09:00Z

Yep, I'll sort it out this evening / this weekend free time depending :)

On Thu, 28 Jul 2016 at 10:58 Carl-Erik Kopseng [email protected]
wrote:

This needs to be rebased to include #1088
#1088, no?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1090 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAMN-QEvtTo78CT1VbZJB0k9S5GekNX2ks5qaH04gaJpZM4JI4nR
.

jonnyreeves · 2016-07-31T12:08:41Z

why did you end up not using util.deprecate()?

a) it doesn't copy the prototype property, so we still need our own util
b) it writes to console.error without first testing for the presence of the console (IE7) and if the console has a func named error.
c) util.deprecate() will only warn the user once; this implemented warns them each time the deprecated func is invoked (not sure which behaviour is desirable - happy to update our custom impl if we decide to stick with it to only warn once as well).

If you feel strongly I can switch it over to use util.deprecate() with this caveats in place (although it does mean we need to fix / update phantomic first).

jonnyreeves · 2016-07-31T12:12:38Z

OK; all functions marked as deprecated in the .md file now write a message to the console when invoked via the sinon object.

I believe the only outstanding questions are around how the deprecate util should work - once we've cleared that up we are good to land this and starting thinking about a cleaner API / internals refactor for sinon v3!!

jonnyreeves · 2016-07-31T12:26:28Z

@fatso83 I was not aware we were dropping support for ES3 (legacy) browsers in 2.x; I'm more than happy to revert this back to using Object.keys and Array.forEach if you want?

fatso83 · 2016-07-31T13:33:53Z

@jonnyreeves: maybe this highlights the fact that we should explicitly define which browser versions we are supporting going forwards? I am quite sure @mroderick said something about not wanting to support < IE10 (or was it IE11?) in some issue, and I replied that I agreed, and that those needing to support older browsers could use the 1.x branch. I was unable to find that discussion right now using the issue tracker search engine right now. I really don't think we should support anything not supporting ES5 going forwards. Preferably, I'd drop everything pre IE11 as that means we could drop sinon-ie in master and stop having issues like #1091.

Dropping browser support should be part of a migration document, I suppose.

fatso83 · 2016-07-31T13:39:37Z

One hint to browser support in master is that our Sauce Labs tests only cover IE 10 and IE 11.

jonnyreeves · 2016-07-31T17:46:30Z

Good call @fatso83; I've updated the README to add both a Saucelabs build status badge and a Saucelabs Browser Matrix badge - you can preview this here.

Looks like the status is not being pulled through at the moment, we may have to tinker. According to the saucelabs guide, we need to:

Add the badge (done)
Run your tests.
Make sure to set a build number, a pass/fail status, and a public, share, or public restricted visibility for every test that runs using the Test Configuration Options.
You'll know that these are set correctly if your tests have a status of Pass or Failed instead of Finished on your dashboard, and that a build number is also displayed.

I don't have a login for saucelabs - would you or @mroderick be able to take a look for me please?

fatso83 · 2016-07-31T18:09:36Z

@jonnyreeves: In my car, on my way home from vacation, so I just sent you the credentials by email.

jonnyreeves · 2016-07-31T20:36:50Z

Cheers @fatso83; added I've pushed a commit to master which exposes the TRAVIS_BUILD_ID to SauceLabs which should in theory make the badges work; they're still looking broken to me - but I'll see if it magically fixes itself over night...

fatso83 · 2016-07-31T20:44:45Z

Sure it's been pushed? Last commit was 4 hours ago.

mantoni · 2016-07-31T22:23:36Z

@jonnyreeves There was a pull request on min-webdriver that should have made this work. The property it's using is TRAVIS_BUILD_NUMBER though while you mentioned TRAVIS_BUILD_ID. Maybe this should be improved?

mantoni · 2016-07-31T22:25:16Z

@jonnyreeves Also please message me / private twitter so I can send you our SauceLabs account.

jonnyreeves · 2016-08-01T18:43:31Z

No idea why the badges aren't showing in the README preview, but if you visit the links directly you will see them in all their glory!

I think that's everything for this particular PR; @fatso83 @mantoni - let me know if you think there's anything else that needs to be addressed?

mantoni · 2016-08-01T20:03:19Z

Nice! But your empty commit is not actually empty. I would also prefer to rebase this commit away before merging. As a Sinon member you should be able to trigger rebuilds in Travis.

mantoni · 2016-08-01T20:05:29Z

@jonnyreeves I mean, the content of the commit seems fine, just the commit name makes no sense 😄

fatso83 · 2016-08-01T20:23:04Z

@jonnyreeves : the badges are showing in the README, btw :)

LGTM, too, btw, so just remove the last commit and merge it at will.

* Add Migration Guide to docs * Mark sinon core utils and sinon.Event as deprecated. * Update `README.md` to provide Browser Support Matrix.

jonnyreeves · 2016-08-01T21:17:05Z

Whoops, sorry about that last commit; accidentally took a non-ruby publish script I was hacking around on along for the ride! 💃

All commits squashed; could someone give it a quick once over and then make it so, please?

fatso83 · 2016-08-01T21:18:39Z

Boom.

mantoni · 2016-08-01T21:19:26Z

👍 @fatso83 You’ve been 5 seconds faster than me 🚀

sephie · 2017-05-29T09:35:36Z

The migration guide is clear about the use of sinon-test. However, The docs for sinon-test do not explain what testCase() actually is intended for, nor can I find any info in any version of the sinon docs (checked the 1.17 docs too)

--edit
found them after all. I'll be on my way. (http://sinonjs.org/releases/v1.17.6/sandbox/)
--edit

This gist gives me my best guess: something about using sinon-test from a describe()
https://gist.github.com/jclevine/a603c0814fbe12649ac9

If someone could explain the exact function I'd be happy to write some docs over at the sinon-test repo.

fatso83 · 2017-05-29T09:49:47Z

@sephie You are totally correct about that omission, and one I was pondering last Friday 😄 I found the previous description for you on the WayBack Machine. AFAIK, you basically use it as a wrapper around the describe block instead of the it blocks and it supports typical pre/post setup.

sephie · 2017-05-29T10:19:46Z

That's another way to do it. 😆 PR is up over at sinon-test

jonnyreeves mentioned this pull request Jul 11, 2016

Sinon 2.0 Release #966

Closed

36 tasks

fatso83 mentioned this pull request Jul 11, 2016

fakeServer is broken with .respond(500) on 1.17.4 #1040

Closed

jonnyreeves self-assigned this Jul 11, 2016

jonnyreeves added the Awaiting Response label Jul 11, 2016

jonnyreeves removed the Awaiting Response label Jul 13, 2016

jonnyreeves added this to the 2.0 milestone Jul 13, 2016

fatso83 reviewed Jul 27, 2016
View reviewed changes

jonnyreeves force-pushed the feature/v2-migration-guide branch from 1315551 to 74eddc6 Compare July 28, 2016 21:11

Add Sinon v2 Migration Guide

c48c505

* Add Migration Guide to docs * Mark sinon core utils and sinon.Event as deprecated. * Update `README.md` to provide Browser Support Matrix.

jonnyreeves force-pushed the feature/v2-migration-guide branch from 66fe46d to c48c505 Compare August 1, 2016 21:14

fatso83 merged commit f074f01 into sinonjs:master Aug 1, 2016

jonnyreeves deleted the feature/v2-migration-guide branch August 1, 2016 21:20

Add v2.0 Migration Guide #1090

Add v2.0 Migration Guide #1090

Conversation

jonnyreeves commented Jul 10, 2016 • edited Loading

mroderick commented Jul 10, 2016

fatso83 commented Jul 11, 2016 • edited Loading

jonnyreeves commented Jul 11, 2016

mantoni commented Jul 11, 2016

fatso83 commented Jul 12, 2016

jonnyreeves commented Jul 13, 2016

jonnyreeves commented Jul 13, 2016

fatso83 commented Jul 14, 2016

jonnyreeves commented Jul 27, 2016

jonnyreeves commented Jul 27, 2016 • edited Loading

mantoni commented Jul 27, 2016

fatso83 commented Jul 27, 2016

mantoni commented Jul 27, 2016

fatso83 Jul 27, 2016

Choose a reason for hiding this comment

jonnyreeves Jul 27, 2016

Choose a reason for hiding this comment

fatso83 commented Jul 28, 2016

jonnyreeves commented Jul 28, 2016

jonnyreeves commented Jul 31, 2016 • edited Loading

jonnyreeves commented Jul 31, 2016

jonnyreeves commented Jul 31, 2016

fatso83 commented Jul 31, 2016

fatso83 commented Jul 31, 2016

jonnyreeves commented Jul 31, 2016 • edited Loading

fatso83 commented Jul 31, 2016

jonnyreeves commented Jul 31, 2016

fatso83 commented Jul 31, 2016

mantoni commented Jul 31, 2016

mantoni commented Jul 31, 2016

jonnyreeves commented Aug 1, 2016

mantoni commented Aug 1, 2016

mantoni commented Aug 1, 2016

fatso83 commented Aug 1, 2016

jonnyreeves commented Aug 1, 2016 • edited Loading

fatso83 commented Aug 1, 2016

mantoni commented Aug 1, 2016

sephie commented May 29, 2017 • edited Loading

fatso83 commented May 29, 2017

sephie commented May 29, 2017

jonnyreeves commented Jul 10, 2016 •

edited

Loading

fatso83 commented Jul 11, 2016 •

edited

Loading

jonnyreeves commented Jul 27, 2016 •

edited

Loading

jonnyreeves commented Jul 31, 2016 •

edited

Loading

jonnyreeves commented Jul 31, 2016 •

edited

Loading

jonnyreeves commented Aug 1, 2016 •

edited

Loading

sephie commented May 29, 2017 •

edited

Loading