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

Throw error on non-string table comment #2126

Merged
merged 3 commits into from Oct 31, 2017

Conversation

Projects
None yet
3 participants
@rizip1
Contributor

rizip1 commented Jun 16, 2017

According to discussion in #2101

When setting table comment to be empty, knex will throw error on non string args

  • add unit tests for all dialects
  • removed retyping to empty string

All the unit tests are the same. Do you handle this type of redundancy somehow or is it ok with unit tests? Theoretically it might be used as integration test, but I do not think this will be right because these comment tests test only single unit of functionality.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jun 26, 2017

@smulesoft docker tests started failing with node 0.12 any ideas how to get it working, maybe those tests could be at least disabled from 0.12?

@smulesoft

This comment has been minimized.

Contributor

smulesoft commented Jun 27, 2017

It's a dockerode issue (the node docker API). Change the version as shown below and it'll pass
sd

@smulesoft

This comment has been minimized.

Contributor

smulesoft commented Jun 28, 2017

@rizip1 Did you check this?
I think you should actually set the version to "~2.4.3" (and if that doesn't work you can check the exact version "2.4.3")

@rizip1

This comment has been minimized.

Contributor

rizip1 commented Jul 2, 2017

Tried both options but then all tests fail. Is there something else I must do when changing the versions?

@smulesoft

This comment has been minimized.

Contributor

smulesoft commented Jul 3, 2017

  • I checked out your branch and I can run all tests locally both with node 0.12 and 6.10. What about you? Can you run them locally?
  • Those are all timeout errors in the afterEach of the same test. Why don't you give it one more trying increasing the timeout in knexfile.js for the postgres test:
    docker: {
      factory:   './postgres/index.js',
      container: 'knex-test-postgres',
      image:     'postgres:9.6',
      database:  'postgres',
      username:  'postgres',
      password:  '',
      hostPort:  '49152',
      client:    'pg',
      timeout:   10 * 1000 // <-- Here, try 15 or 20 seconds (current value is 10 seconds)
    }
@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jul 23, 2017

Please, rebase, hopefully it work against the latest master.

rizip1 added some commits Jun 16, 2017

Throw error on non-string table comment
* add unit tests for all dialects
* removed non-valid comments retyping to ''
@rizip1

This comment has been minimized.

Contributor

rizip1 commented Jul 27, 2017

Done.

@elhigu

I found one more little thing now that I skimmed through this one more time. Now that node 0.12 support was dropped, there shouldn't be need to lock dockerode version.

@@ -34,7 +34,7 @@
"babel-preset-es2015": "^6.13.2",
"chai": "^3.5.0",
"coveralls": "~2.11.1",
"dockerode": "^2.4.3",

This comment has been minimized.

@elhigu

elhigu Jul 27, 2017

Collaborator

I think, this is not necessary anymore, since I dropped node 0.12 support alltogether.

This comment has been minimized.

@smulesoft

smulesoft Jul 27, 2017

Contributor

Yeah maybe you can keep using new minor versions, or at least try again with ~2.4.3 before sticking to 2.4.3.

@elhigu

elhigu approved these changes Oct 31, 2017

@elhigu elhigu merged commit 74f2a03 into tgriesser:master Oct 31, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment