tagomoris / presto-client-node

Distributed query engine Presto client library for node.js
MIT License
125 stars 57 forks source link

Add basic test suite setup #69

Closed MasterOdin closed 1 year ago

MasterOdin commented 1 year ago

Closes #68

This PR adds a basic test setup for the repo that'll run for any push to the master branch as well as for any PR made against it. Currently the tests used are very minimal, focusing just on getting the basics of what a test suite can look like, with my hope that would add additional tests going forward.

tagomoris commented 1 year ago

I'm busy just now and can't see the detail of this change. I'll be able to take time for this later this week. @MasterOdin Please ping me next week if I don't make any comments.

tagomoris commented 1 year ago

The code looks good. But I ran the tests and got timeout errors on my laptop (macOS, Docker desktop is running). This might be the time to fetch the container images, but I had no information about it. And this can happen to many other contributors.

Could you check and modify the default timeout or any other issues about running those on laptops? It may be a solution to run commands to fetch the container images before running jest.

 FAIL  ./index.spec.js (120.813 s)
  ✓ cannot use basic and custom auth (8 ms)
  presto
    ✕ simple query
    ✕ query with error
  trino
    ✕ simple query
    ✕ query with error

  ● presto › simple query

    thrown: "Exceeded timeout of 60000 ms for a hook while waiting for `done()` to be called.
    Add a timeout value to this test to increase the timeout, if this is a long-running test. See https://jestjs.io/docs/api#testname-fn-timeout."

      23 |   var container;
      24 |   var client;
    > 25 |   beforeAll(function(done){
         |   ^
      26 |     new GenericContainer(image)
      27 |       .withExposedPorts(8080)
      28 |       .withWaitStrategy(Wait.forLogMessage('SERVER STARTED'))

      at beforeAll (index.spec.js:25:3)
          at Array.forEach (<anonymous>)
      at Object.<anonymous> (index.spec.js:22:3)
(snip)
MasterOdin commented 1 year ago

I ended up removing the usage of testcontainers in favor of docker-compose as I think the image sizes are a bit frustrating to deal with in terms of getting timeouts right, without just letting jest potentially run for a long time. There's also some issues in that since the presto image only comes in amd64, it runs kind of poorly on mac M machines, which makes it even harder to get it right.

MasterOdin commented 1 year ago

Additionally, see https://github.com/MasterOdin/presto-client-node/actions/runs/5091070215 for what the CI action looks like when it's triggered.

tagomoris commented 1 year ago

This is a great addition to this repository. Thank you!

I confirmed this works well on my laptop after writing docker-compose.yaml. @MasterOdin Could you add docker-compose.yaml with the content below?

version: '3.7'

services:
  presto:
    image: ahanaio/prestodb-sandbox:0.281
    ports:
      - "18080:8080"
  trino:
    image: trinodb/trino:418
    ports:
      - "18081:8080"

And a section to README about how to run tests on local environments? It should notify developers to wait the lines below after docker compose up:

presto-client-node-trino-1   | 2023-06-02T08:12:37.760Z INFO    main    io.trino.server.Server  ======== SERVER STARTED ========
presto-client-node-presto-1  | 2023-06-02T08:13:29.760Z INFO    main    com.facebook.presto.server.PrestoServer ======== SERVER STARTED ========
MasterOdin commented 1 year ago

Whoops, totally meant to include the docker-compose.yml file, sorry! I've also updated the README with a ## Development section that includes running the docker-compose.yml file as well as running the test suite.

tagomoris commented 1 year ago

@MasterOdin Thank you for the contribution!