Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CI support for Oracle 18c (resolves #4888) #4889

Merged
merged 13 commits into from
Dec 15, 2021

Conversation

code-ape
Copy link
Collaborator

@code-ape code-ape commented Dec 13, 2021

Status: CI support working but 18+ tests failing (current result for oracledb is 321 passing, 18 failing, and 61 pending, fixed 16 tests that were initially failing)

Related Issues:

  1. Task: Add CI support for Oracle 18c #4888
  2. Task: All tests to date pass on Oracle 18c #4890

Recommended Merge Style: Squash, commit messages aren't super clear.

Changes:

  1. Moved all setup logic from .travis.yml file into scripts/oracledb-install-driver-libs.sh with help menus and dry-run capability.
  2. Fixed naming in scripts/docker-compose.yml for oracle to match testing automation logic of other databases.
  3. Added oracle to CI integration tests in .github/workflows/integration-tests.yml
  4. Sped up CI integration tests by only installing pg-native when testing it.
  5. Sped up CI integration tests by installing npm packages while database boots.
  6. Fixed CI integration setup logic by not detaching from "wait" container for database setup.
  7. Fixed 16 of 34 known failing tests. (thanks @OlivierCavadenti)
  8. Added temporary failure catch for CI for oracle to allow merge now and cleanup remaining failing tests in another PR.

@code-ape
Copy link
Collaborator Author

code-ape commented Dec 13, 2021

This PR fully addresses getting Oracle working on CI. However, as seen by tests (see here where they were run on my fork https://github.com/code-ape/knex/actions/runs/1571181356) there are dozens of tests failing for Oracle. I leave it to maintainers on how they would like to address these and handle this merge.

@OlivierCavadenti
Copy link
Collaborator

Amazing work ! For tests I suggest to fix some trivial tests here if you can (i will check later too), and skip the other and make an issue to address them immediatly. Maybe some need specific work and time to solve.

@code-ape
Copy link
Collaborator Author

Thanks @OlivierCavadenti. As you've probably guessed, I'm one of those unfortunate Oracle users right now. So happy to help add some stability for it with this 😄

As for fixing tests, to be honest I have exactly zero experience with debugging knex tests. So not sure what is "trivial" and what isn't. To try and help I did create ticket #4890 to track all known failing tests in checklist format with failure summary. So hopefully that should make it easy to screen which issues should be fixed with this PR and which are larger ones to be done separately. Let me know and I'll try to work on them some this week.

@OlivierCavadenti
Copy link
Collaborator

As for fixing tests, to be honest I have exactly zero experience with debugging knex tests. So not sure what is "trivial" and what isn't.

I will quickly check this tests to see what we can do, obvioulsy it's better to fix the more we can.
At first step, we can start to fix the maximum tests we can in this PR, and decide what we do with remaining tests in few days.

Copy link
Collaborator

@OlivierCavadenti OlivierCavadenti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • In script/stress-test we have :
  oracledbxe:
    image: wnameless/oracle-xe-11g
    ports:
      - "31521:1521"
    environment:
      - ORACLE_ALLOW_REMOTE=true

Need a rename ?

  • Need update package.json, especially db:start and db:start:oracle jobs.

  • When I launch "docker-compose -f scripts/docker-compose.yml up --build -d oracledb && docker-compose -f scripts/docker-compose.yml up waitoracledb" I have a warning with the need to remove orphans (old containers need to be purge). Can we put the flag or no need and the user will do it manually ?

  • I have instance of oracledbxe in stress-test\docker.compose.yml (line 17), knex-stress-tests.js (line 166) and reconnect-test-mysql-based-drivers.js (line 150). I think this need a rename too ?

@OlivierCavadenti
Copy link
Collaborator

OlivierCavadenti commented Dec 14, 2021

@kibertoad
Copy link
Collaborator

@code-ape Can you move oracle to a separate workflow or step and mark as optional so that we can merge this now and fix tests as we go?

@code-ape
Copy link
Collaborator Author

@kibertoad sure. Do you want me to split out @OlivierCavadenti's test fix work onto a separate branch?

@kibertoad
Copy link
Collaborator

@code-ape Not necessary, we can merge both together to have less test failures to begin with :)

@code-ape
Copy link
Collaborator Author

Oracle test failure catch working!

@kibertoad kibertoad merged commit bb7de09 into knex:master Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants