Callback or promise so I can wait until visual diff editor closes before moving on to the next test? about approvals.nodejs HOT 7 OPEN

It's a good question...

I could see some benefit to making it more "synchronous" as you explained above. Currently it's just going to verify, throw up the tool - and sadly if you have 100 failures - it gets a little messy.

But going through 100 synchronously feels like it could never end as well.

Not sure how I think about this - maybe a PR that made it configurable would be interesting to explore.

Personally I've never had a problem with them failing - I fix any with p4merge and once all the instances of the open tool windows have been addressed - re-run tests to re-verify (should be good habit anyway).

Thoughts?

from approvals.nodejs.

Interesting -- what platform do you run it from? For me, on mac, each new failure uses the original p4merge window -- so my workflow is run them all with a donothing reporter, then rerun each failure individually and resolve it with p4merge. But maybe there's something I can change about either my p4merge setup on my machine, or the way the p4merge reporter delegates to p4merge in mac mode.

The other part is that I like having the "resolve" command on my clipboard... it's not that bad to type it in the terminal by reading p4merge titles and using tab completion, but it feels like an unnecessary step. I'm also using this on a project that is in it's baby stages, so there are often trivial UI changes that I want to approve. My guess is once things stabilize I won't have quite so many to approve all the time.

So if the mac/p4merge thing doesn't go anywhere, here's what I was thinking for API changes:

In GenericDiffReporterBase.js on line 66 (spawn(exe, [received, approved], {), we can save the return value from spawn -- it's an instance of ChildProcess (except in tests where a fake spawn is injected). Instead of returning nothing from report(), we can return a Promise that resolves when the ChildProcess' exit event is fired.

That Promise could be passed through and returned by each of the verify methods. So then it's fully opt-in: if you want to wait for the reporter to be closed, you do:

it(...., function(done) {
  approvals.verify(.....)
    .then(() => {
      done();
    });
});

from approvals.nodejs.

From a testing perspective, I was thinking we could write a unit test that uses a custom reporter that calls to a process that waits for some UDP message before terminating. Then we assert that the returned promise has not resolved yet, send the message to that process to terminate, and then assert that the promise has resolved. waits 2 seconds before terminating. The test would be something like

  it(...., function(done) {
    let didResolve = false;
    verify(....).then(() => { didResolve = true });
    expect(didResolve).to.be.false
    setTimeout(() => expect(didResolve).to.be.false, 1500);
    setTimeout(() => expect(didResolve).to.be.true, 2500);
    setTimeout(() => done(), 2501);
  });

from approvals.nodejs.

+1 to something. I just tried approvals for the first time, and 300+ meld sessions spawning back to back nearly crushed my computer!

from approvals.nodejs.

To follow up here...

@mattgodbolt We've thrown a some updates into a beta build of approvals at v4.0.0-beta.2 to help with the 300+ meld sessions... Give it a look

@alexanderbird - in doing some thinking I like the idea of verify returning a promise - but didn't get that in yet. There are a few knobs to play with at the config level that can help you now though (if you use a difftool that is more synchronous like opendiff or p4merge).

from approvals.nodejs.

👍🏻 Thanks for the update!

from approvals.nodejs.

@staxmanade Looks great: thanks!

from approvals.nodejs.