P5.js: grunt tests occasionally fail

[lmccart]

rerunning grunt works again. this happens both with travis ci and locally

[outofambit]

this seems to reliably come from grunt mocha:yui, in the first example for loadJSON. my guess is that its because this example loads JSON from an external web resource (the USGS Earthquake API) and the request fails for some external reason. (i'm able to reproduce the failure by turning my laptop's wi-fi off.)

[kadamwhite]

Not sure how applicable they are here but we could consider using a library like VCR or Replay to save a snapshot of that response locally, to allow the tests to pass offline

[outofambit]

i'm looking at https://github.com/seglo/grunt-connect-prism. seems like it wouldn't be too hard to integrate into our infrastructure

[meiamsome]

If this is just an intermittent network issue we could add retries(5) to the test so it can try again?

[outofambit]

@meiamsome that's a good short term solution, but in the longer term I think the tests should pass even when there is no internet access on the machine.

@sakshamsaxena let me know if there's any way i can help!

[meiamsome]

@outofambit I totally agree. It looks to me like the real issue here is that loadJSON doesn't call self._decrementPreload() when the load fails - is there some defined intended behaviour for failures in preloads? If not then it's kind of undefined behaviour when this fails, so maybe the test should not be considered a failure?

[sakshamsaxena]

Yep I'm looking into it now. I worked a lot with grunt this summer, so I believe I've developed a good understanding and likeability for it, and also the patience to deal with such random failures :laughing: :sweat_smile: I'll keep ya posted!

[sakshamsaxena]

Found new errors while I was on Windows. See #2209

Still working on isolating other errors meanwhile.

[sakshamsaxena]

Here's what I've figured out till now :

On Windows, Files p5.prototype.loadTable CSV files should handle escaped quotes and returns within quoted fields is a constant error due to the difference of \r\n and \n which fails the assertion.
Also on Windows, if we run tests in actual browser, then except above, no failures are reported. However, running in console, new p5() / global mode is triggered when "draw" is in window is another constant failure. Tried to isolate this, and turns out it's most probably due to Phantom. This is also the error which is occasionally thrown in other OS and Travis too.

Meanwhile, loadJSON example is up for debate, whether to keep the source local or on internet.

Once the loadTable error is resolved, we can close this and move discussion over to #2174 as that would most probably be a good lead.

[meiamsome]

I think new p5() / global mode is triggered when "x" is in window tests are a bit flawed. They seem very susceptible to race conditions where the page load is triggered before the relevant function is added to the window object. It could probably do with a rewrite where the function is passed into createP5Iframe and then written to the window object prior to p5. I might have a look later today to see if I can work on that.

[sakshamsaxena]

@meiamsome It's what I used to think, but isn't reliably so. I tried to replace setup call with draw, and it failed, and when I tried to replace draw with setup, it didn't. Multiple times. Both teardown and createP5Iframe are robust and perfectly sync, and work flawlessly in browser. It is at the CLI I'm facing the issue.

Let me know what you find ! 😄

[meiamsome]

So, it looks like the problem is that multiple done calls count as a failure. I've changed the test to the following, and this appears to resolve the issue (I'm just have the tests in a while loop until they fail - and it's still going well without failure.)

    test('is triggered when "draw" is in window', function() {
      return new Promise(function(resolve, reject) {
        createP5Iframe();
        iframe.contentWindow.draw = function() {
          resolve();
        };
      });
    });

Also acceptable:

    test('is triggered when "draw" is in window', function(done) {
      createP5Iframe();
      var complete = false;
      iframe.contentWindow.draw = function() {
        if(!complete) {
          done();
        }
        complete = true;
      };
    });

It'd be good if you can test to confirm/deny that this fixes the issue

[sakshamsaxena]

Gee! This does work like a charm! But I don't understand _why_ this particular task fails? Why the done() calls are getting messed up here ? It felt like the draw function itself may have some scope of improvement too, personally (?) But great work @meiamsome ! 😄

[meiamsome]

@sakshamsaxena It's just that mocha is designed to consider a test with multiple dones as a failure (Logical failure of the test, that is). The iframe will call draw every frame, so mocha decides it has failed on the second frame. Promises on the other hand are designed that only the first resolve/reject actually matters.

(I just realised I could've got rid of the pointless wrapping function, but whatever)

[sakshamsaxena]

Let's observe the Travis builds till Sunday, and then we'll satisfactorily close this as resolved !

[meiamsome]

@sakshamsaxena Sadly, this is not entirely fixed (See #2265 ) - we still need to resolve the issue with examples requesting data from remote services.

[outofambit]

taking a run at integrating https://github.com/seglo/grunt-connect-prism this weekend