Dockest should by default stop containers when jest process exists

[leventov]

Dockest doesn't stop containers after an unexpected quit by default:

🌈 Dockest 🌈 DockestServices running, running Jest 

  ●  process.exit called with "1"

      33 |     } catch (err) {
      34 |         fastify.log.error(err);
    > 35 |         process.exit(1);
         |                 ^
      36 |     }
      37 | })();
      38 | 

      at src/web.js:35:17
🌈 Dockest 🌈 [Exit Handler] {
  "trap": "exit",
  "code": 1
}

 RUNS  test/web.test.js
error Command failed with exit code 1.

So that I have to sweep them manually via docker-compose down.

Describe the solution you'd like Stop/remove containers anyway.

[erikengervall]

Odd, this should be catched by

https://github.com/erikengervall/dockest/blob/ade90b9ed9d6999456822ef170786abd1f3d6b65/packages/dockest/src/run/bootstrap/setupExitHandler.ts#L101-L120

Unless the setupExitHandler is never invoked 😕

[n1ru4l]

could it be possible because of https://github.com/erikengervall/dockest/blob/ade90b9ed9d6999456822ef170786abd1f3d6b65/packages/dockest/src/run/bootstrap/setupExitHandler.ts#L105 being an async function and therefore not executed immediately?

[erikengervall]

could it be possible because of

https://github.com/erikengervall/dockest/blob/ade90b9ed9d6999456822ef170786abd1f3d6b65/packages/dockest/src/run/bootstrap/setupExitHandler.ts#L105

being an async function and therefore not executed immediately?

That could definitely be the case.

I'm not sure why there's an async at the handler-level, it could probably be removed without any impact. exitHandler has to be async though in order to teardown resources 😕

[n1ru4l]

Yeah, process.exit() shouldn't be called anyways. It is much more safe to set process.exitCode = 1 and free the event loop.

@leventov Could you provide some more input on why you call process.exit, as it seems to be inside your application test logic.

See documentation: https://nodejs.org/api/process.html#process_process_exit_code

If it is necessary to terminate the Node.js process due to an error condition, throwing an uncaught error and allowing the process to terminate accordingly is safer than calling process.exit()

Throwing an un-catched error e.g. setTimeout(() => { throw err }, 0) could be the better solution.. The process.on("uncaughtException") handler should then be responsible for clearing the event loop and setting process.exitCode to 1 so the app can exit (un-)gracefully without force-exiting.

---

Maybe spawning jest as a child process would prevent the dockest code to exit as well?

[leventov]

Indeed, I think the reason was that I called process.exit() directly and did use runInBand.

If there is nothing dockest could do about this perhaps there could be a notice somewhere in the docs that this shouldn't be done.

FWIW, anyway, I think that not stopping containers (neither upon abrupt exit nor normal test completion) should be the default and the preferred strategy, to avoid excessive restarts and faster testing loop.

[erikengervall]

Describe the solution you'd like Stop/remove containers anyway.

---

Indeed, I think the reason was that I called process.exit() directly and did use runInBand.

If there is nothing dockest could do about this perhaps there could be a notice somewhere in the docs that this shouldn't be done.

FWIW, anyway, I think that not stopping containers (neither upon abrupt exit nor normal test completion) should be the default and the preferred strategy, to avoid excessive restarts and faster testing loop.

Firstly, these two statements appear to be in conflict of one another.

---

I'd like to learn more about your use-case so that I can better understand this issue. As I explained in the other issue, dockest primarily focuses on reliability rather than e.g. speed.

If your use-case is local development, I'd suggest either

• using docker-compose up as is
• using dockest with debug enabled which "Pauses Dockest just before executing Jest. Useful for more rapid development using Jest manually"

Both of these alternatives puts the responsibility of running Jest on the user, meaning the result of Jest won't affect the containers.

[leventov]

Do you intend dockest primarily for usage in CI? If I'd better use docker-compose up locally, why cannot the same be done in CI? Is it health checks / readiness checks that is the difference from vanilla docker-compose?

[erikengervall]

Do you intend dockest primarily for usage in CI?

The initial purpose for dockest was to replace tedious/flaky bash scripts for managing containers. It has thus far focused primarily on being reliable and less flaky than its bash-counterparts (described here).

I would however love to see dockest make local development less painless as well and welcome suggestions on how to achieve this or contributions towards that goal with open arms 🙏🏼

If I'd better use docker-compose up locally, why cannot the same be done in CI? Is it health checks / readiness checks that is the difference from vanilla docker-compose?

If your type of services are trivial, you could definitely just run docker-compose up, sleep for an arbitrary time and hope that resources are ready once tests run.

Dockest helps with establishing both connectivity and readiness after which one can execute startup commands for services before tests run (i.e. database migrations and whatnot). Other than that it also exposes a bunch of configuration and also outputs contextual information during setup and teardown.

[machbio]

@erikengervall dont you think there should be an option for re-runing the tests after failed jest error without having to manually stop and remove docker containers

"dockest": "^2.0.2",

Docker-compose containers are not stopped after test failure in jest.

[n1ru4l]

@machbio Could you please share your dockest configuration file?

[machbio]

const { Dockest, logLevel } = require('dockest')

const dockest = new Dockest({
    composeFile: ['docker-compose-postgres.yml'],
    dumpErrors: true,
    debug: false,
    jestLib: require('jest'),
    jestOpts: {
        bail: true,
        ci: true,
        forceExit: true,
        verbose: true,
    },
    logLevel: logLevel.DEBUG,
})

dockest.run([
    {
        serviceName: 'postgres-service',
        commands: ['npm run migrate up'],
        readinessCheck: async ({
            defaultReadinessChecks: { postgres },
            dockerComposeFileService: {
                environment: { POSTGRES_DB, POSTGRES_USER },
            },
        }) =>
            Promise.all([
                (new Promise((resolve) => {
                    setTimeout(resolve, 50)
                    // eslint-disable-next-line no-console
                    console.log('Arbitrary ReadinessCheck step')
                }),
                postgres({ POSTGRES_DB, POSTGRES_USER })),
            ]),
    },
])

[machbio]

@n1ru4l docker-compose containers are not stopped and removed after test cases in jest fails.

I have also tried changing around jestOpts - all combinations of true and false for the above parameters fails to stop the docker-compose container

version: '3.7'

services:
    postgres-service:
        image: postgres:10.4
        ports:
            - published: 35432
              target: 5432
        environment:
            POSTGRES_DB: db
            POSTGRES_USER: user
            POSTGRES_PASSWORD: pass
            POSTGRESS_PORT: 5432

[machbio]

even with an exitHandler with process.exit(0) - the docker containers persist on failed jest runs

    exitHandler: function () {
        process.exit(0)
    }

[machbio]

@n1ru4l Is there anything I could do to help you identify the issue ?

[n1ru4l]

Sorry currently not spending that much time on dockest and still had no time to upgrade to version 2 as I still need to finish https://github.com/erikengervall/dockest/pull/167

[machbio]

Ok, let me know if the following analysis makes any sense -

Trying to understand Teardown logic, dockest run - https://github.com/erikengervall/dockest/blob/86aaca845f81f0a889afa5338b8f1c170be41e8e/packages/dockest/src/index.ts#L65-L66

Jest success is recorded here - https://github.com/erikengervall/dockest/blob/86aaca845f81f0a889afa5338b8f1c170be41e8e/packages/dockest/src/run/runJest.ts#L21-L23

Teardown Single does not consider the value of success to teardown the container https://github.com/erikengervall/dockest/blob/86aaca845f81f0a889afa5338b8f1c170be41e8e/packages/dockest/src/run/teardown.ts#L19

So i added some debug statements to see the difference between success and failure jest, seems like it fails at this condition and teardown does not continue - is dockest not handling some change in jest runcli function ? https://github.com/erikengervall/dockest/blob/86aaca845f81f0a889afa5338b8f1c170be41e8e/packages/dockest/src/run/runJest.ts#L17

    jestLib: require('jest'), \\ dockest jestlib configuration
    "jest": "^25.2.7", \\ jest package.json version used

[carlrygart]

@machbio I'm pretty sure it's because of the bail: true. Remove it and it will for sure work. In your case, jest will enforce process.exit(1) immediately upon the first failing test, and therefore the exitHandler is never called.

@erikengervall @n1ru4l I think I've read that the process.on('exit', fn) listener only can perform synchronous operations as you say. However, I'm not sure if it's going to work in this case anyway. Jest is running is multiple processes as default right? So the listener will not work since jest itself is doing the exit. Correct me if I'm wrong 🙂

[machbio]

@carlrygart you are right - removing bail worked (bail: false) - thank you