Immediate Return Option

[machineghost]

In a test environment any kind of delay can be inconvenient, because it means interrupting the normal test runner flow. That in turn means that any related logic which fails will fail outside the test's scope, leaving the test runner unable to properly report those failures. However, even if one uses mockjax it is difficult to avoid this, because mockjax (almost) always runs asynchronously, even with a timeout of 0.

This problem has a simple solution: make mocked AJAX requests return immediately (ie. synchronously). However, the only way to do that in mockjax right now is to specify "async: false" ... in the production code. When a developer only wants to run synchronously in a test environment, having to change production code somewhat defeats the purpose of using a mock.

It would be very handy if there was a way to tell mockjax "pretend this AJAX request returned immediately, so I don't have to interrupt my test flow", and there easily could be with only two small changes to mockjax's code ...

if ( requestSettings.async === false) {
    // TODO: Blocking delay
    process();
} else {
    this.responseTimer = setTimeout(process, parseResponseTimeOpt(mockHandler.responseTime) || 50);
}

to:

if ( requestSettings.async === false || mockHandler.async === false) {
    // TODO: Blocking delay
    process();
} else {
    this.responseTimer = setTimeout(process, parseResponseTimeOpt(mockHandler.responseTime) || 50);
}

and:

(function(mockHandler, requestSettings, origSettings, origHandler) {
    mockRequest = _ajax.call($, $.extend(true, {}, origSettings, {

to:

(function(mockHandler, requestSettings, origSettings, origHandler) {
    if (mockHandler.async === false) {
        origSettings.async = false;
    }
    mockRequest = _ajax.call($, $.extend(true, {}, origSettings, {

I hope you will consider this idea, as I think it would be a great improvement to an already great tool.

[jakerella]

@machineghost While you are correct that even a responseTime of 0 causes an sync action within Mockjax, that is intentional. If we were to force an XmlHttpRequest call to be synchronous in the way you mention we could unintentionally change how the application under test operates because the calling code would be expecting the request to be asynchronous. While in your case it might not matter, creating such an option, in my opinion, would be antithesis to the idea of unit testing because it would fundamentally change the execution flow of the application JavaScript.

I'd like to hear if anyone else has opinions here... @dcneiner or @jcreamer898 ?

[dcneiner]

I agree @jakerella – we looked into doing this in the past (See discussion in issue #104). At the time I was OK with attempting it, but there is code within jQuery we'd have to work around as well. At this point, when testing Asynchronous code, I'd recommend writing the tests in such a way where you pause execution then resume when complete. Most unit testing frameworks support this.

[machineghost]

Thank you for considering my proposal, but I would like to clarify that I am not advocating for a change the default behavior of mockjax, only for a 100% optional way to achieve something that is otherwise impossible.

As for the point about test frameworks being able to handle deferred code, that is both correct and incorrect. Take Mocha (which I believe to be one of the most robust frameworks available). Mocha does provide a way to do asynchronous testing, and it looks like this:

it('should work after the response comes back', function(done) {
    $.ajax(options).done(function() {
        assertSomething();
        done();
    });
});

However, there are three major downsides: 1) the synchronous version of that code is much simpler:

it('should work after the response comes back', function() {
    $.ajax(options);
    assertSomething();
});

2) any errors that occur inside the callback can't be traced back to the test, so if anything goes wrong when we assertSomething Mocha will report that as an "unattached test failure" (and we'll have no idea which test caused that failure)

3) asynchronous tests take longer to run, and in a unit test suite speed is important

So to recap I'm not trying to change how mockjax works, I'm not advocating that everyone should use my proposed option, and I do understand there are trade-offs to making normally asynchronous code behave synchronously. All I'm saying is, if what you're trying to test is makeAnAjaxCall(); assertTheCorrectThingHappened() there are some real, tangible benefits to being able to do that synchronously, and currently it is impossible for a mockjax user to realize those benefits. Wouldn't a completely optional feature, that let's users do something otherwise impossible which really can improve their code, be a beneficial addition?

P.S. To dcneiner's point that "there is code within jQuery we'd have to work around as well", I really think that the two changes I suggested are all that is necessary. This is because my approach wasn't to modify jQuery's behavior at all, instead I simply:

1) changed a single point of mockjax's mock replacement code 2) updated the options that are passed to jQuerys code to include async: false

1) clearly doesn't involve jQuery, and 2) is "black box" in the sense that it just relies on jQuery's existing mechanism for requesting synchronous AJAX.

[jakerella]

@machineghost You make good points, and there are certainly benefits to having synchronous code (speed and simplicity for example), but to your other points, #2 in particular is disconcerting. Your test framework (whatever it is) should be able to handle this use case. In fact, a simple stack trace would easily show where things were broken if an Error were thrown, for example. QUnit handles this without a problem (not that I'm recommending you switch to that, I am not). The point is, the tests not reporting where assertions failed correctly in asynchronous tests is a bigger problem.

As for your point about the feature being optional, you are correct that people wouldn't have to use it, but I am more concerned about people using it inappropriately. I go back to my earlier point which is that it is entirely possible that such a feature would cause unforeseen issues in the source application code due to the async action returning immediately. If application code were expecting an async action and the test did not deliver on that promise (no relation to actual "Promises") then the test code would not be accurately mocking what would happen in production.

At this point I don't think we're willing to introduce a feature that fundamentally switches such a core aspect of XMLHtppRequests, even on an optional basis. That said, if this works for you, forking the code and switching those lines is a great option, and we would encourage that!

Good luck.

[machineghost]

Fair enough. I guess all I can say is:

At this point I don't think we're willing to introduce a feature that fundamentally switches such a core aspect of XMLHtppRequests, even on an optional basis.

This feature wouldn't fundamentally switch anything; all it would do is provide a way to set a jQuery option. That's it. There's no changing of anything, no new anything, just passing a "key:value" pair to jQuery from the test code.

In fact, a simple stack trace would easily show where things were broken if an Error were thrown, for example.

Please, before you give up on this feature, just try this simple experiment. You can literally copy/paste the code into your developer console:

function a() {
    b();
}
function b() {
    window.setTimeout(function() {
        throw new Error();
    });
}
a();

and tell me if you see "a" in the stack trace. If you can show me any test framework, QUnit or otherwise, that can give a proper stack trace from the above code I will completely admit that I am wrong and that there is no use for this feature

... but I can pretty much guarantee that you won't, because this is a fundamental limit of the languagte itself, not a flaw in any particular test framework..

[jakerella]

I see how your example would not be captured by a stack trace, point taken. That said, you are asking for a feature that does fundamentally change how XMLHttpRequest works by switching an otherwise async action to a synchronous one. Yes, it is an option, but it is an option that the application code did not set... intentionally. So it would be altering, fundamentally, how the application code executes. In my opinion, this has the potential to cause more issues than it solves. Even providing the option is an issue because some people will just think "oh, my tests will run faster!" And not realize that it fundamentally changes how they're source code executes.

But like I said, there's no reason you can't fork the code and add the option if you need it!

[machineghost]

Yes, it is an option, but it is an option that the application code did not set

The goal of MockJax is to help users test AJAX, correct? And to help your users do their testing, MockJax modifies the very internals of jQuery's code ... which is a good thing. But for MockJax to simply pass an argument along from one place in the code to another place it can't reach ... is bad?

It seems to me you are worried about users who are:

• intelligent enough to be doing Javascript testing in the first place
• intelligent enough to understand how a mocking system replaces production code
• too stupid to understand the difference between synchronous and asynchronous code

Because you fear that they will blindly will use an option they don't understand (despite documentation warning them not to), on the basis that it will make their tests run faster (even though no documentation, anywhere, will give them that idea).

I would suggest that the user I just described doesn't exist. But users who are serious about JavaScript testing, who have hundreds if not thousands of tests, and lose hours (or more) regularly when they have to track down an unattached test failure ... those users can't even have the option of a solution?

fork the code and add the option if you need it

Is there any way I could possibly get you to reconsider? Bribe you with a gift basket or a couple of those nice frozen Omaha steaks or something? Just because something is different from what you are used to doesn't make it bad. Sure I can fork the library, but then I have to create (and more importantly, maintain) the entire fork just so I can change < 10 lines to pass one simple option, and then no one but me gets to benefit from this change.

[jakerella]

And to help your users do their testing, MockJax modifies the very internals of jQuery's code ... which is a good thing.

Actually, we do not modify the internals of jQuery, we mock it out. There's a big difference, primarily in that our users expect Mockjax to behave exactly as jQuery.ajax() would. That means that if the application code specifies an ajax call should be asynchronous, then Mockjax needs to make it asynchronous.

But for MockJax to simply pass an argument along from one place in the code to another place it can't reach ... is bad?

Here's the problem: the argument isn't being passed by the application code, it's being passed by the test code. The test code should not alter the application code. Thus, for us to accept an option from the tests that does (sorry, there is no argument here) fundamentally alter how the application code will execute is in fact bad.

I'm sorry, but unless my cohorts (@dcneiner and @jcreamer898 currently) overrule me on this it isn't an option that I'm willing to add in.

As to your evaluation of the type of user I'm referring to, your focusing on the wrong piece of the argument.

[dcneiner]

@machineghost @jakerella – Sorry guys, I've not been home this week so haven't stayed up with this thread. I'll catch up and respond – hopefully tomorrow. I do want to mention one small feature in Chrome Stable – if you open the "Sources" tab, and check the "Async" checkbox, you will see a in the stack trace so it can remain attached to the calling code.

[dcneiner]

OK – finally got to spend some time on this @machineghost @jakerella

So first, an immediate solution is available without forking or modifying Mockjax. @machineghost mentioned Mocha, so I'll use that as the example. In a global (or local) before and after pair:

before( function () {
   $.ajaxSetup({ async: false });
});
after( function () {
   $.ajaxSetup({ async: true });
});

This will make all ajax requests run synchronously. You can of course use this technique on a per test or more granular basis.

Now, as to adding an async: false option to Mockjax. I see the arguments from @machineghost as good points but I am reluctant to encourage (or even provide a built in way for) test suites to completely ignore actual behavior. I think we should at the least add documentation about how to use the code I put above for those who really want synchronous test suites, perhaps including information on Chrome's async handler. The primary concern is that due to the different way of writing sync vs. async tests, it means the end user developer or QA team can not optionally run their tests as async later if they were written as sync to begin with.

It would appear the best way to use sync specs would be to write all tests as async (with Mocha's done handlers, etc), then use the before/after code above to make the tests sync. You could then provide a way to run then one way or the other - sync for the bulk of development, but async before deployments to verify all the code still works in normal conditions.

[machineghost]

@dcneiner That's a great suggestion, but it only works if you want all of your tests to run synchronously. The thing is though, even though most of the time a developer won't care about the asynchronicity of a call, sometimes they might. For instance, if you have a call that triggers an AJAX action, then an immediate action, you might want to test that the immediate action occurs first even though it comes after the AJAX code, which would require async: true.

One could do the reverse, and only use ajaxSetup to explicitly turn off asynchronicity for the desired tests. That would certainly be a better solution, but it still has some disadvantages; consider:

beforeEach( function () {
    $.ajaxSetup({ async: false });
});
afterEach( function () {
    $.ajaxSetup({ async: true });
});
describe('SomeClass', function() {
    it('can do something AJAX-related', function() {
        var mock = sinon.stub(SomeClass.prototype, 'postAjaxMethod');
        $.mockjax({
            data: {test: true},
            url: '/foo'
        })
        new SomeClass().someAjaxMethod();
        expect(mock.args[0][0].test).to.be(true);
    });
});

vs.

describe('SomeClass', function() {
    it('can do something AJAX-related', function() {
        var mock = sinon.stub(SomeClass.prototype, 'postAjaxMethod');
        $.mockjax({
            async: false,
            data: {test: true},
            url: '/foo'
        })
        new SomeClass().someAjaxMethod();
        expect(mock.args[0][0].test).to.be(true);
    });
});

Not only is the latter shorter, but it keeps the "this call has been abnormally mocked to be synchronous" bit right with the other mocking code, instead of putting it in the beforeEach/afterEach. This can be relevant if the suite contains a number of tests, separating the beforeEach/afterEach code by hundreds of lines from the test code.

The primary concern is that due to the different way of writing sync vs. async tests, it means the end user developer or QA team can not optionally run their tests as async later if they were written as sync to begin with.

Well, everyone organizes their tests differently, but I'm not clear on why anyone would want to do that. Think of it this way: tests that use {async: flag} as just another form of mock, and just as you wouldn't want to rip out your mocks and run your unit tests with real objects, you wouldn't want to change asynchronous: false code to suddenly be asynchronous one day. And if you did, you'd be ripping out the $.mockjax code also, which (if async was part of Mockjax and not a separate beforeEach/afterEach) would remove the async: false.

[jakerella]

@machineghost First off, we hope that you understand that we appreciate the discussion and suggestion and that we honestly do want to make Mockjax a tool that fits the bill for Ajax testing in as many cases as possible. That said, we feel that there are multiple potential workarounds or alternatives, and unless there is some overwhelming community support for such a feature we are not going to implement it.

I'm going to close the issue for now, but we can reopen in the future if need be.

[machineghost]

Fair enough. I think I made my case for the feature as clearly as possible and no further discussion will improve it. However, if the value of mock-only synchronicity isn't apparent to you now, I can still hope that it will become apparent in the future. If you keep writing asynchronous tests you will no doubt hit a "failure not associated with any test" at some point, because they are possible in any test framework, and once you do experience the pain of them you might better appreciate the value of this feature. Similarly if you ever get brave and experiment with using synchronicity in your AJAX tests, I think the benefits of that approach will quickly become obvious to you.

So, here's hoping you do more AJAX testing, and perhaps get curious about whether the grass is greener on the other side of the fence.

[jakerella]

@machineghost
Wow, that was a very condescending reply. Whether you intended it to be or not.

"...here's hoping you do more AJAX testing..."

I have written more JavaScript tests than most people have lines of JavaScript code. There's a reason that we wrote libraries like this one to help with our tests, it's because we believe that testing significantly improves our end product. And the fact that you would imply that we don't understand the benefits of various testing strategies, or that we haven't properly considered a proposed option is beyond disrespectful, it is blatantly aggressive.

We have tried to see things from all sides, and we (not just me, but multiple people, including some not commenting on this thread) have all decided that this particular feature would not be prudent. These are people with individual experience in JavaScript dating back to the birth of the language. Your choice of statements is not welcome, and I advise you to choose your words much more carefully if you intend to be a part of our open source community.

[machineghost]

I really should have started by saying this, but since I didn't let me to say it now: Mockjax has been a very valuable tool for me, and I absolutely appreciate the time and effort of everyone who has contributed to it. While I do disagree strongly with the team on this particular issue, I never intended to be disrespectful in any way, and I apologize if I was unintentionally.

It absolutely was not my intent to be condescending, but unfortunately plain text has a way of losing context, and because of my carelessness it definitely was lost here. The key part of what I tried, but failed, to communicate was "if the value of mock-only synchronicity isn't apparent to you now, I can still hope that it will become apparent in the future."

With absolutely no insult intended, I genuinely didn't/don't understand how anyone who has "written more JavaScript tests than most people have lines of JavaScript code" could have possibly never run in to the "unattached error" issue. No major test framework that I've worked with (including QUnit, JS Test Driver, and Mocha) solves the issue, so it honestly seemed to me that anyone who hadn't seen it either:

A) used a less popular test runner that associates callback errors with their originating tests B) had no real logic in their callbacks C) hadn't done enough testing to run in to it

I ruled out A) because no one here ever said "we use test runner X which solves this", and I ruled out B) because anyone who bothers to write an AJAX test tool probably has logic in their callbacks. By Occam's Razor that left only C), so I assumed that was the reason you hadn't experienced the pain.

Evidently I was wrong; either I ruled out A) or B) prematurely or I failed to consider option D, that you haven't seen this problem for some other reason. Either way, I never meant in any way to insult anyone's capability or experience as a programmer. I simply was trying (but failed) to express the idea that I hoped that you would see the value of this feature in time because whatever factor (A/B/C or D) was preventing it would change.

P.S. I should also say that (unlike some OSS projects) I felt everyone here made an effort to fairly consider this feature, and not simply reject it without cause. My sentiment of hope that you would see things differently in the future relates to this: because I felt the feature was denied fairly based on the current circumstances, I couldn't possibly hope for you to re-evaluate it "more fairly", I could only hope that your circumstances/experiences would change and in doing so change your evaluation.

[juanmendes]

Voting for async: false setting. A synchronous test is much simpler to maintain and I don't need setup and teardowns

[jakerella]

@juanmendes Thanks for the vote, and for participating in the conversation, but the opinion of the core contributors is that this would not be a wise addition. Making mock ajax calls no longer asynchronous when they were specified as such in the source code could easily cause false positive (or negative) test results.

We will not be implementing this feature, but you can always do it in a fork if you feel it is necessary.

[juanmendes]

@jakerella I did read the conversation and only posted because it was said that if there was enough support from the community, you may consider it.

I have been using synchronous mocking of AJAX calls for a long time (and also have written more lines of JavaScript test code than most have written lines of JavaScript :smiley: ) and have never encountered a false positive or negative because of making the calls synchronous in tests. Have you actually had problems or are you simply being extra cautious?

[jakerella]

Yes, I have had this bite me before. When you switch a function's execution from being async to sync things can happen out of expected order in your source code. This can cause (or remove) race conditions that do/don't actually exist in the source. In other words, those can be missed by the tests.

This is not just my opinion, it is that of the other contributors and a few other devs I've asked. Our thought is that this is a bad practice. It fundamentally changes the way the XMLHttpRequest was created, not just mocking, but changing the execution path and strategy of the JS engine.

[machineghost]

@juanmendes the maintainers of this library have made it clear that they don't care about community feedback on this issue. They've also made it clear that they won't improve things for JS professionals with real-world use cases so long as there is any chance that some moron developer who doesn't understand what asynchronicity means might shoot themselves in the foot by using a feature they don't understand.

_shrug_\ It's their library and their call. My advice is to just use Sinon as much as possible to eliminate the need for mockjax in unit tests, and then use Selenium when you actually need to test real world timing stuff.

[juanmendes]

@machineghost I think they do care. I think they are just being a bit overzealous. I'm just hoping maybe others will ask for the same feature too. Your flaming response won't help our cause though :smile:

[jakerella]

@juanmendes Thanks for the vote of confidence. Seeing as how some folks can't keep things polite I'll have to lock this issue.