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

Jasmine clock + Promise unintuitive #1659

Closed
chrisjdev opened this issue Feb 28, 2019 · 12 comments
Closed

Jasmine clock + Promise unintuitive #1659

chrisjdev opened this issue Feb 28, 2019 · 12 comments

Comments

@chrisjdev
Copy link

chrisjdev commented Feb 28, 2019

It took me a while to figure out how to test a function that used a Promise and a timeout with Jasmine.clock(), so I figured I'd log this issue with my solution and a request to change something. I'm basically reposting issue #710 and #1282 and asking for a better solution than "this is more of a how-to-unit-test question rather than a jasmine issue".
An example simplified function to unit test:

import { NEVER, from, Observable } from 'rxjs';
import { flatMap, timeout } from 'rxjs/operators';
const myFuntion = (): Observable<void> => {
    return from(Promise.resolve()).pipe(
        flatMap(() => Promise.resolve()),
        flatMap(() => NEVER.pipe(timeout(30000))));
};

And the unit test (nonworking):

describe("Jasmine clock + Promise", function() {
    it('should timeout with Promise+clock', (done: DoneFn): void => {
        jasmine.clock().install();
        myFuntion().subscribe(
            () => {
                done.fail('was supposed to timeout error');
            },
            (error) => {
                expect(error.name).toEqual('TimeoutError');
                done();
            }
        );
        jasmine.clock().tick(30000);
    });
});

Expected Behavior

tick() or some other function should trigger the timeout.

Current Behavior

jasmine.clock().install() disables timing out until tick is called. Tick must be called after the timeout is set, which may be set in an async context and inaccessible from the unit test.

Possible Solution

Replace the tick() line with this:

Promise.resolve().then(() => Promise.resolve()).then(() => jasmine.clock().tick(30000));

Or, mark the function provided to it() as async and replace the subscribe( line with:

await await myFuntion().subscribe(

Alternately, as mentioned by the other issues and StackOverflow post, override Promise to be synchronous, or save off the original setTimeout() function before installing jasmine clock. Perhaps Jasmine could:

  • offer an option to make Promises synchronous
  • mention that Promises are asynchronous in the jasmine.clock() documentation
  • perhaps when jasmine.clock() is installed, that could be a reference time, so if it was ticked forward, it would then trigger any timeouts that should've been triggered, even if those timeouts were set after the jasmine.clock().tick() call.
  • perhaps a new method like jasmine.clock().tick(300).withAsyncLevels(3) that would result in the code
        Promise.resolve().then(() => {
            Promise.resolve().then(() => {
                Promise.resolve().then(() => {
                    jasmine.clock().tick(30000);
                });
            });
        });

Context

I spent multiple days trying to figure out why my code appeared to be ceasing execution in the chain of promises and observables I was attempting to test, eventually narrowing it down to an issue between interactions of Promise + Jasmine + Observable timeout operator. Then learning that jasmine.clock().tick() won't trigger timeouts that haven't been set yet, Promises are defined to be always async, and Observables may be async or sync depending on how they're used.

Your Environment

  • Version used: 2.99.1
  • Environment name and version (e.g. Chrome 39, node.js 5.4): Chrome 72
  • Operating System and version (desktop or mobile): macOS
@slackersoft
Copy link
Member

Promises resolving asynchronously is even more complicated than you indicate here. Most promise libraries actually use a few other tricks to do asynchronous callbacks for resolution and rejection with less overhead that setTimeout(fn, 0).

If I'm understanding you correctly, I think Jasmine actually does do some of the things that you suggest. If you schedule a setTimeout during a tick, the callback for that setTimeout should be executed before tick returns. There is a caveat on setTimeout(fn, 0) if that occurs on the last millisecond of the current tick, but you should be able to work around that by ticking for an extra one. An example:

describe('timeouts', function() {
  beforeEach(() => { jasmine.clock().install() });
  afterEach(() => { jasmine.clock().uninstall() });

  it('does some stuff', function() {
    var thing1 = jasmine.createSpy('thing1');
    var thing2 = jasmine.createSpy('thing2');

    thing1.and.callFake(() => setTimeout(thing2, 0));
    setTimeout(thing1, 0);

    expect(thing1).not.toHaveBeenCalled();
    expect(thing2).not.toHaveBeenCalled();

    jasmine.clock().tick(0);

    expect(thing1).toHaveBeenCalled();
    expect(thing2).toHaveBeenCalled();
  });
});

This fails because thing2 isn't called. But

describe('timeouts', function() {
  beforeEach(() => { jasmine.clock().install() });
  afterEach(() => { jasmine.clock().uninstall() });

  it('does some stuff', function() {
    var thing1 = jasmine.createSpy('thing1');
    var thing2 = jasmine.createSpy('thing2');

    thing1.and.callFake(() => setTimeout(thing2, 0));
    setTimeout(thing1, 0);

    expect(thing1).not.toHaveBeenCalled();
    expect(thing2).not.toHaveBeenCalled();

    jasmine.clock().tick(1);

    expect(thing1).toHaveBeenCalled();
    expect(thing2).toHaveBeenCalled();
  });
});

This passes. This functionality is the current defined functionality for the Jasmine clock (we even have a comment denoting):

              // checking first if we're out of time prevents setTimeout(0)
              // scheduled in a funcToRun from forcing an extra iteration

The reasoning for this seems to me to be so you can tick and check the side effects from the first callback without having any subsequent callbacks run. I could also see allowing an option to tick to proceed through any setTimeout(0)s that occurred, but I wouldn't want to make this the default functionality right now.

Hope this helps. Thanks for using Jasmine!

@chrisjdev
Copy link
Author

Thank you very much for the response. I hadn't considered behavior with setTimeout(0). I still believe that having to tick the clock forward after any Promises resolve async requires too much knowledge of the code under test, and of how Jasmine works. I believe Promises are implemented without setTimeout(), so they run async and the Jasmine clock won't change that. Also, writing a unit test that locks in how many Promises are involved in the code under test means that if future code changes increases the number of Promises in the future, it will start to fail the unit test, even though it's working as intended. Or worse, if it was previously synchronous and not using a DoneFn, it could fail an unrelated unit test, or pass when it should've failed.

Please let me know if there's anything I can clarify.

@slackersoft
Copy link
Member

I agree that you don't want to write a spec that knows about the promise depth in the code under test. That being said, if you're testing code that uses promises you probably want to use the new-ish promise support in Jasmine (either async/await or returning a Promise) to ensure that your specs are waiting for a promise to resolve. I don't think it will make sense for Jasmine to provide an way to do anything else to promises in the near future, if only because we still support browsers that don't have Native promises so we would have to figure out how to do it for any promise library.

I would also be happy to review a pull request for the docs to give an example of providing a promise implementation that completes synchronously/on demand.

@chrisjdev
Copy link
Author

chrisjdev commented Mar 21, 2019

I was looking at Jasmine code, and it looks like scheduleFunction called by the fake setTimeout/setInterval functions does not have a branch to call the function immediately, which is fair, because I imagine you wouldn't want code like this to call thing1:

describe('timeouts', function() {
  beforeEach(() => { jasmine.clock().install() });
  afterEach(() => { jasmine.clock().uninstall() });

  it('does some stuff', function() {
    var thing1 = jasmine.createSpy('thing1');
    jasmine.clock().tick(1);
    setTimeout(thing1, 0);
    expect(thing1).not.toHaveBeenCalled();
  });
});

To make testing async code a little easier, one option would be to have a new method so thing1 would be called in that case:

describe('timeouts', function() {
  beforeEach(() => { jasmine.clock().install() });
  afterEach(() => { jasmine.clock().uninstall() });

  it('does some stuff', function(done) {
    var thing1 = jasmine.createSpy('thing1');
    // new function call, records current clock value for reference
    jasmine.clock().markTimeForAsync(); 
    jasmine.clock().tick(1);
    // fake setTimeout sees it is 1 past the reference time, and runs thing1 immediately
    setTimeout(thing1, 0);
    expect(thing1).toHaveBeenCalled();
    // Another example, this would work also
    var thing2 = jasmine.createSpy('thing2');
    jasmine.clock().markTimeForAsync();
    Promise.resolve(() => setTimeout(thing2, 0)).then(() => {
        expect(thing2).toHaveBeenCalled();
        done();
    });
    jasmine.clock().tick(1);
  });
});

I don't think I was previously aware of the async / await or returning a Promise options for specs...those do sound useful, but it still wouldn't cover some of our use-cases where functions we're unit testing are triggering things that happen asynchronously and don't return a Promise.

@slackersoft
Copy link
Member

It sounds like what you're looking for is a way for a previous call to tick to be going to trigger the call of a setTimeout that might happen in the future. I'm not entirely sure how that could work in a way that would be intuitive. I think we might also be losing some of the nuance with the samples here, so I'm having a hard time seeing what the real use case for something like this is. Another note is that I don't think your example with thing2 would work even without a mock clock because the Promise library would probably enqueue the then callback in a way that preempts the setTimeout, which makes the Jasmine clock behavior possibly more realistic.

@chrisjdev
Copy link
Author

Yes, one way to make Jasmine clock + Promise more intuitive would be for a previous call to tick trigger a call of a setTimeout that might happen in the future. I'm also not sure what the most intuitive way for that to happen would be...my proposal was for a new function called markTimeForAsync() to enable that behavior to avoid breaking tests that aren't expecting that behavior. I'm certainly open to other ideas. Is there some way I can clarify our use-case for something like this? We've got functions that don't return anything and trigger behavior asynchronously, that I tried to boil down in the original bug report. I was hoping for a way to test that the function interacts with other objects as intended through spies and using jasmine.clock() to avoid waiting for a full 30s timeout. Currently, I can do that with

        Promise.resolve().then(() => {
            jasmine.clock().tick(30000);
        });

but if there's another Promise added to the source code in the future, that will break. And I find it to be unintuitive that I need to advance the clock like that in the first place.

@slackersoft
Copy link
Member

The Jasmine clock is fairly complicated already, and I don't really see a good way to add functionality like this without surprising other users while still being able to figure out what the clock is going to do in any given circumstance. I would either see what kind of use you can make out of actually awaiting some of the portions of your code instead of forcing all of the asynchronous work to be synchronous with clock, or listen to your tests that are telling you this is hard to use and see if you can find another way to architect the system that the tests are easier to write.

Hope this helps. Thanks for using Jasmine!

@chrisjdev
Copy link
Author

I just realized that the code in the original issue wasn't failing as I said it was, so I fixed it and made the example slightly more complicated to illustrate my issue. As far as our use-case, we've got a auto-login function that needs to load some items saved in Storage. We've got the Storage interface mocked out with Promises that resolve, and HTTP mocked out with Observables. If you're loading a username, server name, and authentication token, then performing a HTTP call to check if it's working, then you've got multiple async things and a timeout on the last item.

Thank you very much for at least considering my issue. I really appreciate the work you do with Jasmine. You can close this issue if you don't think it's worth addressing.

@slackersoft
Copy link
Member

Unfortunately, I can't really think of a good way to incorporate some of this logic without impacting the ability to reason about the clock. I could see maybe adding a note to the documentation to clock about interactions with promises, but the asynchronous nature of promises is something that I would expect users using promises to know on their own, since this isn't due to anything that Jasmine is doing.

I'm going to close this. But please feel free to open a PR for a note that seems like it makes sense either here in the JSDoc comments or in the documentation repo.

@chrisjdev
Copy link
Author

I learned a little bit more about Jasmine, RxJS's delay operator, and Angular. Specifically, if I used "delay()" from RxJS, it would never fire even if I had jasmine.clock() installed and I used jasmine.clock().tick() to advance the clock past where it should fire. It turns out that "delay()" looks for a scheduled Date to fire as well as using setInterval(). Since jasmine.clock().tick() doesn't advance the clock unless you also call jasmine.clock().mockDate(), it never fires. In trying to figure out what I was doing wrong for that issue, I found Angular's fakeAsync(), tick(), and flushMicrotasks() functions. Those seem to work the way I was hoping Jasmine would work (tick() will trigger Promises to resolve as well as timers as well as advance Date.now()).

@mlntdrv
Copy link

mlntdrv commented Jul 19, 2021

For faking RxJS time, you can use the 2nd argument of delay() which is a SchedulerLike and pass it a default value of asyncScheduler in your production code. This will run real time in your production code. In the test you can substitute the asyncScheduler with a VirtualTimeScheduler which fakes the time. Exercising the VirtualTimeScheduler::flush() method will advance the time enough that all scheduled delays and timers will fire.

@chrisjdev
Copy link
Author

I didn't know about the VirtualTimeScheduler, thank you for bringing that up. I think I'd still prefer cleaner helper functions, rather than spying on RxJS operators to substitute the VirtualTimeScheduler. Also, a new scheduler for RxJS wouldn't address the issues with Promises.

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

No branches or pull requests

3 participants