wrap column names in quotes for numeric & hyphen based column names

[rubengmurray]

No description provided.

[danvk]

Hi @rubengmurray, thanks for the contribution! Could you add some tests for this? A unit test and also an integration test?

For general cleanliness of the output, it would be nice to only quote properties that really need it.

[rubengmurray]

Completely forgot about this @danvk - I've added it to my todos to circle back on when I can.

[rubengmurray]

@danvk when I run the test suite locally (on master branch) I'm getting this test report:

Test Suites: 3 failed, 3 total
Tests:       15 failed, 8 passed, 23 total
Snapshots:   0 total
Time:        3.239 s
Ran all test suites.

Can you confirm?

[rubengmurray]

@danvk when I run the test suite locally (on master branch) I'm getting this test report:

Test Suites: 3 failed, 3 total
Tests:       15 failed, 8 passed, 23 total
Snapshots:   0 total
Time:        3.239 s
Ran all test suites.

Can you confirm?

Haven't done what the README says. I will take a look actually 👀

[rubengmurray]

yeah I'm still seeing the same report via the yarn README steps

[danvk]

Hi @rubengmurray -- what errors are you getting? How are you running the tests? Do you have an instance of Postgres running that the tests can connect to? If not, that would explain why most of the tests are failing.

The tests are failing on CI because the snapshots haven't been updated. You can update snapshots with yarn test -u.

Just to reiterate what I said earlier:

For general cleanliness of the output, it would be nice to only quote properties that really need it.

Here's how I run the tests locally and the output that I see:

$ POSTGRES_URL=postgres://danvk@localhost:5432/postgres yarn test
yarn run v1.22.11
$ jest
 PASS  src/__tests__/typescript.test.ts
 PASS  src/__tests__/schemaPostgres.test.ts
 PASS  test/integration/integration.test.ts (6.571 s)
  ● Console

    console.log
      opopopopop

      at Object.<anonymous> (test/integration/cli-test.ts:25:13)

    console.log
      opopopopop

      at Object.<anonymous> (test/integration/cli-test.ts:43:13)

    console.log
      opopopopop

      at Object.<anonymous> (test/integration/cli-test.ts:60:13)

    console.log
      Type [path has been mapped to [any] because no specific type has been found.

      at pgTypeToTsType (src/schemaPostgres.ts:82:15)

    console.log
      Type [path has been mapped to [any] because no specific type has been found.

      at pgTypeToTsType (src/schemaPostgres.ts:82:15)


Test Suites: 3 passed, 3 total
Tests:       31 passed, 31 total
Snapshots:   19 passed, 19 total
Time:        7.168 s
Ran all test suites.
✨  Done in 8.89s.
POSTGRES_URL=postgres://danvk@localhost:5432/postgres yarn test  21.73s user 2.57s system 267% cpu 9.076 total

[rubengmurray]

Yeah, definitely have postgres locally as I use it for many other projects. Have passed the environment variable in your syntax with my local credentials and I'm still seeing all the same errors:

 FAIL  src/__tests__/typescript.test.ts
  ● TypeScript › generateTableInterface › empty table definition object

    TypeError: options.transformTypeName is not a function

      56 |   return x !== null && x !== undefined;
      57 | }
    > 58 |
         | ^
      59 | export interface TableNames {
      60 |   var: string;
      61 |   type: string;

      at Object.generateTableInterface (src/typescript.ts:58:29)
      at Object.<anonymous> (src/__tests__/typescript.test.ts:11:57)

Interestingly, this seems to be a difference between the outputted typescript.js

function generateTableInterface(tableNameRaw, tableDefinition, options) {
    var tableName = options.transformTypeName(tableNameRaw);
    var selectableMembers = '';
    var insertableMembers = '';

vs .ts

export function generateTableInterface(
  tableName: string,
  tableDefinition: TableDefinition,
  schemaName: string,
  options: Options,
): [code: string, names: TableNames, typesToImport: Set<string>] {
  let selectableMembers = '';
  let insertableMembers = '';
  const columns: string[] = [];
  const requiredForInsert: string[] = [];
  const typesToImport = new Set<string>();

I'm not really familiar with jest, I've only really used mocha, but it seems it's not testing against the .ts but the .js which is out of sync. I've run tsc but the files aren't updating.

Update: not really sure where those artefacts came from but I've deleted them and I'm now at

Test Suites: 1 failed, 2 passed, 3 total
Tests:       3 failed, 28 passed, 31 total
Snapshots:   16 passed, 16 total
Time:        5.5 s

I'll continue to work through.

And your point about For general cleanliness of the output, it would be nice to only quote properties that really need it: I'm aiming to use the tests to prove a change to this effect 👍

[rubengmurray]

Looks now like it's failing because of an in-built Postgres function which I don't have access to. It appears to have been added in Postgres 13, but I'm running 12 locally (https://pgpedia.info/g/gen_random_uuid-function.html)

Might need a docker-compose adding with Postgres 13 to spin up for the integration tests.

$ jest
 PASS  src/__tests__/typescript.test.ts
 PASS  src/__tests__/schemaPostgres.test.ts
 FAIL  test/integration/integration.test.ts
  ● Console

    console.log
      Type [path has been mapped to [any] because no specific type has been found.

      at pgTypeToTsType (src/schemaPostgres.ts:82:15)

    console.log
      Type [path has been mapped to [any] because no specific type has been found.

      at pgTypeToTsType (src/schemaPostgres.ts:82:15)

  ● Integration tests › Schema generation › pg-to-sql features

    error: function gen_random_uuid() does not exist

      at Parser.parseErrorMessage (node_modules/pg-protocol/src/parser.ts:369:69)
      at Parser.handlePacket (node_modules/pg-protocol/src/parser.ts:188:21)
      at Parser.parse (node_modules/pg-protocol/src/parser.ts:103:30)
      at Socket.<anonymous> (node_modules/pg-protocol/src/index.ts:7:48)

  ● Integration tests › Schema generation › prefixWithSchemaNames generation

    error: function gen_random_uuid() does not exist

      at Parser.parseErrorMessage (node_modules/pg-protocol/src/parser.ts:369:69)
      at Parser.handlePacket (node_modules/pg-protocol/src/parser.ts:188:21)
      at Parser.parse (node_modules/pg-protocol/src/parser.ts:103:30)
      at Socket.<anonymous> (node_modules/pg-protocol/src/index.ts:7:48)

  ● Integration tests › Schema generation › pg-to-sql with camelCase

    error: function gen_random_uuid() does not exist

      at Parser.parseErrorMessage (node_modules/pg-protocol/src/parser.ts:369:69)
      at Parser.handlePacket (node_modules/pg-protocol/src/parser.ts:188:21)
      at Parser.parse (node_modules/pg-protocol/src/parser.ts:103:30)
      at Socket.<anonymous> (node_modules/pg-protocol/src/index.ts:7:48)

Test Suites: 1 failed, 2 passed, 3 total
Tests:       3 failed, 28 passed, 31 total
Snapshots:   16 passed, 16 total
Time:        5.5 s

[danvk]

Thanks for digging into this @rubengmurray. Your docker-compose idea makes sense to me. I'd suggest removing the UUIDs since they're somewhat incidental to the test and I didn't realize they required Postgres 13. But I use UUIDs quite extensively on my work project, so I actually kinda like having them tested.

Could you add a note to the README saying that running the tests requires Postgres 13. And if you make a Dockerfile or docker-compose command that sets this up, that'd be lovely to commit as well.

[rubengmurray]

I'll see if i can get around to doing the docker bit over the next few days.

[rubengmurray]

I'm actually not too familiar with jest so finding it more difficult to debug this in general on the test runs. I've made a bit of progress on adding the docker tests to the repo but nothing that runs without a hitch at this point. I'll keep it on my radar to complete at some point but can't make any promises on time. Up to you @danvk whether you want to leave this open/running as a draft.

As an aside, I'm using this package now (based on your previous changes) as part of an automated script, post knex migration, which keeps all types up-to date on every database change. Very happy with this.