Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sinon.resetHistory() does not reset history #2022

Merged
merged 4 commits into from
May 21, 2019
Merged

sinon.resetHistory() does not reset history #2022

merged 4 commits into from
May 21, 2019

Conversation

rpgeeganage
Copy link
Contributor

@rpgeeganage rpgeeganage commented May 15, 2019

Purpose (TL;DR) - mandatory

Fix issue: #2016 sinon.resetHistory() does not reset history

Remove createStubInstance from sinon.js so same method from sandbox will be used.

How to verify - mandatory

No tests added yet.
But followed below code snippet.

const sinon = require('../../my_sinon_fork/sinon/');
const { assert } = require('referee');

describe('resetHistory', function() {
  var num = null;

  beforeEach(function() {
    num = sinon.createStubInstance(Number);
  });

  afterEach(() => {
    // Restore the default sandbox here
    sinon.restore();
  });

  describe('num.toFixed.resetHistory()', function() {
    it('num.toFixed.resetHistory()', function() {
      num.toFixed();
      assert.isTrue(num.toFixed.called);
      num.toFixed.resetHistory();
      assert.isFalse(num.toFixed.called);
    });
  });

  describe('sinon.resetHistory()', function() {
    it('sinon.resetHistory()', function() {
      num.toFixed();
      assert.isTrue(num.toFixed.called);
      sinon.resetHistory();
      assert.isFalse(num.toFixed.called);
    });
  });
});

Need to execute as follows.
mocha test.js
Got the following output.

  resetHistory
    num.toFixed.resetHistory()
      ✓ num.toFixed.resetHistory()
    sinon.resetHistory()
      ✓ sinon.resetHistory()


  2 passing (27ms)

Checklist for author

  • npm run lint passes
  • References to standard library functions are cached.

@coveralls
Copy link

coveralls commented May 15, 2019

Pull Request Test Coverage Report for Build 2854

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 94.208%

Totals Coverage Status
Change from base Build 2842: 0.0%
Covered Lines: 1656
Relevant Lines: 1724

💛 - Coveralls

@rpgeeganage
Copy link
Contributor Author

@fatso83 ,
Thank you for code snippet to recreate the bug.
The sample code in the https://runkit.com/fatso83/5cd3e47cdb83d1001a88c4c6 has a small issue.
(It may be I executed it wrong)
The problem is when I run node test.js, beforeEach block doesn't execute for each test.
I tested code in the description with Mocha library and got the expected output.
Please give you feedback on this.

@rpgeeganage rpgeeganage changed the title removed the createStubInstance method fonr sinon.js sinon.resetHistory() does not reset history May 15, 2019
@fatso83
Copy link
Contributor

fatso83 commented May 16, 2019

@rpgeeganage Basically you found a bug in mini-mocha: it's just a toy I hacked together for demo purposes in a small hour, so it turns out there are a few kinks here and there (like missing async support).

If things work using Mocha, then that's your verification :-)

@rpgeeganage
Copy link
Contributor Author

@fatso83 ,
I like the mini-mocha lib 😄. I may use it for my small stuff too. If you need help with extending that library, I'm happy to lend you a hand.

Finally, I verified the output using the main Mocha library and added to the description. Are you ok with the changes?
What kind of a test case do you prefer for this ?

@fatso83
Copy link
Contributor

fatso83 commented May 16, 2019

Of course I'd love some help on mini-mocha!. It's quite useful for verifying issues without needing a full development environment and it would be quite useful to link to in a issue template, once it's a bit more feature complete (like async support).

With regards to the test case, I think just adding a simplified version of the last test above to the file https://github.com/sinonjs/sinon/blob/master/test/issues/issues-test.js would be sufficient. Something like describe("2016", function(){ it("it should add instance stubs to the default sandbox" where you just instantiate some Foo prototype.

@rpgeeganage
Copy link
Contributor Author

@fatso83 ,
Thanks for the information. I'll update the test.

@rpgeeganage
Copy link
Contributor Author

rpgeeganage commented May 16, 2019

@fatso83,
I have added tests.
(I was just wondering, How someone become a maintainer for Sinon library someday?
is there any procedure for that?)

@fatso83
Copy link
Contributor

fatso83 commented May 20, 2019

@rpgeeganage We invite the ones who stick around, adding pull requests and seem interested in contributing to the project :-)

@rpgeeganage
Copy link
Contributor Author

@fatso83 ,
Thanks for the information. I'll do more contributing to this project in the future :)
Are you ok with the tests?

@fatso83 fatso83 merged commit ff505fc into sinonjs:master May 21, 2019
@fatso83
Copy link
Contributor

fatso83 commented May 21, 2019

Thank you!

@rpgeeganage
Copy link
Contributor Author

I'm honored to work in this repo.

@rpgeeganage rpgeeganage deleted the fix/sinon_reset_history branch May 21, 2019 12:24
kevinbosman added a commit to kevinbosman/sinon that referenced this pull request Aug 14, 2019
PR sinonjs#2022 redirected sinon.createStubInstance() to use the Sandbox
implementation thereof. This introduced a breaking change due to the
sandbox implementation not supporting property overrides.

Instead of duplicating the original behaviour from stub.js into
sandbox.js, call through to the stub.js implementation then add all
the stubs to the sandbox collection as usual.

Copy the missing tests from stub-test.js
kevinbosman added a commit to kevinbosman/sinon that referenced this pull request Aug 14, 2019
PR sinonjs#2022 redirected sinon.createStubInstance() to use the Sandbox
implementation thereof. This introduced a breaking change due to the
sandbox implementation not supporting property overrides.

Instead of duplicating the original behaviour from stub.js into
sandbox.js, call through to the stub.js implementation then add all
the stubs to the sandbox collection as usual.

Copy the missing tests from stub-test.js and add issue tests.
mroderick pushed a commit that referenced this pull request Sep 2, 2019
PR #2022 redirected sinon.createStubInstance() to use the Sandbox
implementation thereof. This introduced a breaking change due to the
sandbox implementation not supporting property overrides.

Instead of duplicating the original behaviour from stub.js into
sandbox.js, call through to the stub.js implementation then add all
the stubs to the sandbox collection as usual.

Copy the missing tests from stub-test.js and add issue tests.
franck-romano pushed a commit to franck-romano/sinon that referenced this pull request Oct 1, 2019
* removed the createStubInstance method fonr sinon.js
* added test
franck-romano pushed a commit to franck-romano/sinon that referenced this pull request Oct 1, 2019
PR sinonjs#2022 redirected sinon.createStubInstance() to use the Sandbox
implementation thereof. This introduced a breaking change due to the
sandbox implementation not supporting property overrides.

Instead of duplicating the original behaviour from stub.js into
sandbox.js, call through to the stub.js implementation then add all
the stubs to the sandbox collection as usual.

Copy the missing tests from stub-test.js and add issue tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants