upAll doesn't fail when docker-compose failed to up

[deser]

Circumstances:
docker for windows hasn't been started

Usage:

What I see in console:

Expected behavior:

1. promise returned by upAll is rejected

[AlexZeitler]

@deser I created a rough fix for it in #75 but currently it breaks our tests due to an odd behaviour in docker-composes usage of stderr and stdout, see docker/compose#5590 for details.

@Steveb-p What are your thoughts on this?

[Steveb-p]

@AlexZeitler looks good to me. It's exactly the kind of behavior that exit code reliance should fix.

EDIT: I'm looking confused at the CI errors :)

EDIT2: It seems that exit event is emitted before last data event? I'm not sure, gonna take a look at it running on my local machine.

[AlexZeitler]

@Steveb-p Sorry haven't got an update notification for your edit updates 😞

Looking at the failing test ensure exit code is returned correctly and https://github.com/PDMLab/docker-compose/blob/master/index.js#L69, I guess there's a conceptual issue between the result + exitCode and Promises rejecting.

[Steveb-p]

@AlexZeitler Actually, I've checked this again after your comment. Apparently, there is nothing wrong with how EventEmitter for child processes behaves in relation to promises. It's just that now tests actually do reject, which causes awaits in tests to throw errors (since that's how async/await is supposed to work.

Previously they would just always resolve.

I'm in the process of reworking tests to take it into account. Also, I'm changing the reject() argument to result object instead of only exit code to allow working with stdout/stderr outputs if needed.

[Steveb-p]

@AlexZeitler I'm working on it and migrating it to Jest, but kinda got stuck on some Docker issues for building. And apparently some building tests check wrong services - which I've noticed only after I've started migrating.

If you're gonna look on it further tomorrow (since it's 1:00 here and I have to go to work early in the morning and not be a worthless zombie), you'd probably want to change in your PR at the very least:
index.js (execCompose function)

  childProc.on('exit', exitCode => {
    result.exitCode = exitCode;
    if (exitCode === 0) {
      resolve(result);
    } else {
      reject(result);
    }
  });

So it rejects with stderr and output still available and without wrapping it with Error object.
Original in your PR for your reference:

  childProc.on('exit', exitCode => {
    result.exitCode = exitCode;
    if (exitCode !== 0) {
      return reject(new Error(result.err));
    }

    return resolve(result);
  });

In test/index.js, where you have:

assert.equal(1, (await compose.logs('non_existent_service', { cwd: path.join(__dirname) })).exitCode);

it should be replaced with something similar to:

  try {
    await compose.logs('non_existent_service', { cwd: path.join(__dirname) });
    assert.true(false, 'Execution should not reach this point');
  } catch (result) {
    assert.equal(result.exitCode, 1);
  }
  await compose.down({ cwd: path.join(__dirname), log: true });
  assert.end();

(also, notice missing compose.down call, which caused issues further down the line with orphan containers).

[AlexZeitler]

@Steveb-p That's what I noticed as well and tried and also tried to fix it.
Does your refactoring towards jest include the upAll fixes required for this issue or would you rely on my fix (which still has broken tests)? If you're refactoring towards jest I guess it doesn't make much sense for me to fix the tape based tests for now. But I'm not sure we should release my fix then until tests are stable again.

[Steveb-p]

My jest based test crashed when building images due to docker errors (due to those tests running in parallel, which caused docker to conflict out operations). Otherwise I don't recall any other important issues.

I'll publish the PR in the evening when I have access to the laptop with relevant branch on.

EDIT: I'm pretty sure I based that branch on yours.

[Steveb-p]

#77 should properly solve this case.

0.19 version of this package should now reject in this case.

[Steveb-p]

@AlexZeitler I believe it should be closed, unless it still doesn't work in 0.19/0.20.

@deser try if it works for you, if possible.

[deser]

Thanks, seems it is

[AlexZeitler]

Great, I'll close this then...