Miscellaneous improvements in unit tests. #608

Merged
merged 3 commits into from Jul 6, 2014

Conversation

2 participants
@gurjeet
Contributor

gurjeet commented Jun 15, 2014

End statements with semicolons, to be consistent with the surrounding
code.

Added a new unit test to ensure environment variables are honored when parsing a
connection string.

Added a TODO to cleanup a test that emits messages using console.log().

Correct a query's syntax. Looks like a good thing to do even though the syntax
doesn't matter in mocked out tests.

Removed a test that tests for SELECT tags; AFAIK, SELECT commands don't emit a
tag.

gurjeet added some commits Jun 15, 2014

Miscellaneous improvements in unit tests.
End statements with semicolons, to be consistent with the surrounding
code.

Added a new unit test to ensure environment variables are honored when
parsing a
connection string.

Added a TODO to cleanup a test that emits messages using console.log().

Correct a query's syntax. Looks like a good thing to do even though the
syntax
doesn't matter in mocked out tests.

Removed a test that tests for SELECT tags; AFAIK, SELECT commands don't
emit a
tag.
@brianc

This comment has been minimized.

Show comment Hide comment
@brianc

brianc Jun 17, 2014

Owner

Very nice - thank you sir.

Just as an aside: The tests are a bit of a mess honestly - they were made mostly in a time before things like mocha or node-tap existed. I have considered a few times porting them to a nicer test framework than my home grown makefile rube-goldberg machine, but I can't bring myself to do a major refactor of the tests at this point. I approached almost the entire development in a TDD style way and now that they're passing I think it's too risky to rip them all out and plop them into a more sophisticated framework. They've also accumulated a lot of small syntax ugliness over the years as other people have contributed, etc. I also neglect style guidelines a bit there because "Hey I'm already writing tests - I'm doing the right thing! I'm gonna relax a bit." So...thanks for the cleanup. 😄

One problem: one of the tests is now failing. Would you be able to fix that failing test? Afterwards I'll merge 'em all in.

Owner

brianc commented Jun 17, 2014

Very nice - thank you sir.

Just as an aside: The tests are a bit of a mess honestly - they were made mostly in a time before things like mocha or node-tap existed. I have considered a few times porting them to a nicer test framework than my home grown makefile rube-goldberg machine, but I can't bring myself to do a major refactor of the tests at this point. I approached almost the entire development in a TDD style way and now that they're passing I think it's too risky to rip them all out and plop them into a more sophisticated framework. They've also accumulated a lot of small syntax ugliness over the years as other people have contributed, etc. I also neglect style guidelines a bit there because "Hey I'm already writing tests - I'm doing the right thing! I'm gonna relax a bit." So...thanks for the cleanup. 😄

One problem: one of the tests is now failing. Would you be able to fix that failing test? Afterwards I'll merge 'em all in.

@gurjeet

This comment has been minimized.

Show comment Hide comment
@gurjeet

gurjeet Jun 17, 2014

Contributor

I work for EnterpriseDB, and you can say I have a vested interest in making
sure Postgres tooling is in good shape :) I am working on it in my spare
time, so can't dedicate the amount I would like to.

I have updated code in commit 23b29c9. Unfortunately Travis build failed,
and it appears that it's a timing issue in the tests.

I ran the npm test on my machine, and that also failed the first 2 times,
but at different places both the times. I ran 3 tests after those and each
one of the last 3 tests have passed.

So I think the branch is ready for merge, and because it's just test case
cleanups, you can feel free to merge it in any of the stable branches too.

The two failed tests ended with:

***Testing Pure Javascript***
130-tests.jsMessage: expected Error: Command failed: psql: FATAL:  role
"gurjeet" does not exist
 to be null
AssertionError: expected Error: Command failed: psql: FATAL:  role
"gurjeet" does not exist
 to be null
    at Function.assert.isNull
(/home/gurjeet/dev/NODE-POSTGRES/test/test-helper.js:170:10)
    at
/home/gurjeet/dev/NODE-POSTGRES/test/integration/gh-issues/130-tests.js:10:16
    at /home/gurjeet/dev/NODE-POSTGRES/test/test-helper.js:159:16
    at ChildProcess.exithandler (child_process.js:641:7)
    at ChildProcess.EventEmitter.emit (events.js:98:17)
    at maybeClose (child_process.js:743:16)
    at Process.ChildProcess._handle.onexit (child_process.js:810:5)

 AssertionError: expected Error: Command failed: psql: FATAL:  role
"gurjeet" does not exist
 to be null
    at Function.assert.isNull
(/home/gurjeet/dev/NODE-POSTGRES/test/test-helper.js:170:10)
    at
/home/gurjeet/dev/NODE-POSTGRES/test/integration/gh-issues/130-tests.js:10:16
    at /home/gurjeet/dev/NODE-POSTGRES/test/test-helper.js:159:16
    at ChildProcess.exithandler (child_process.js:641:7)
    at ChildProcess.EventEmitter.emit (events.js:98:17)
    at maybeClose (child_process.js:743:16)
    at Process.ChildProcess._handle.onexit (child_process.js:810:5)

xargs: node: exited with status 255; aborting
make: *** [test-integration] Error 124
npm ERR! Test failed.  See above for more details.
npm ERR! not ok code 0

And:

api-tests.js (binary)..........
heroku-pgpass-tests.js (binary).Message: Expected execution of function to
be fired within 5000 milliseconds NaN to change timeout globally)
AssertionError: Expected execution of function to be fired within 5000
milliseconds NaN to change timeout globally)
    at null._onTimeout
(/home/gurjeet/dev/NODE-POSTGRES/test/test-helper.js:138:12)
    at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)

 AssertionError: Expected execution of function to be fired within 5000
milliseconds NaN to change timeout globally)
    at null._onTimeout
(/home/gurjeet/dev/NODE-POSTGRES/test/test-helper.js:138:12)
    at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)

xargs: node: exited with status 255; aborting
make: *** [test-binary] Error 124
npm ERR! Test failed.  See above for more details.
npm ERR! not ok code 0

On Tue, Jun 17, 2014 at 10:11 AM, Brian C notifications@github.com wrote:

Very nice - thank you sir.

Just as an aside: The tests are a bit of a mess honestly - they were made
mostly in a time before things like mocha or node-tap existed. I have
considered a few times porting them to a nicer test framework than my home
grown makefile rube-goldberg machine, but I can't bring myself to do a
major refactor of the tests at this point. I approached almost the entire
development in a TDD style way and now that they're passing I think it's
too risky to rip them all out and plop them into a more sophisticated
framework. They've also accumulated a lot of small syntax ugliness over the
years as other people have contributed, etc. I also neglect style
guidelines a bit there because "Hey I'm already writing tests - I'm doing
the right thing! I'm gonna relax a bit." So...thanks for the cleanup. [image:
😄]

One problem: one of the tests is now failing. Would you be able to fix
that failing test? Afterwards I'll merge 'em all in.

Reply to this email directly or view it on GitHub
#608 (comment).

Gurjeet Singh http://gurjeet.singh.im/

EDB www.EnterpriseDB.com http://www.enterprisedb.com

Contributor

gurjeet commented Jun 17, 2014

I work for EnterpriseDB, and you can say I have a vested interest in making
sure Postgres tooling is in good shape :) I am working on it in my spare
time, so can't dedicate the amount I would like to.

I have updated code in commit 23b29c9. Unfortunately Travis build failed,
and it appears that it's a timing issue in the tests.

I ran the npm test on my machine, and that also failed the first 2 times,
but at different places both the times. I ran 3 tests after those and each
one of the last 3 tests have passed.

So I think the branch is ready for merge, and because it's just test case
cleanups, you can feel free to merge it in any of the stable branches too.

The two failed tests ended with:

***Testing Pure Javascript***
130-tests.jsMessage: expected Error: Command failed: psql: FATAL:  role
"gurjeet" does not exist
 to be null
AssertionError: expected Error: Command failed: psql: FATAL:  role
"gurjeet" does not exist
 to be null
    at Function.assert.isNull
(/home/gurjeet/dev/NODE-POSTGRES/test/test-helper.js:170:10)
    at
/home/gurjeet/dev/NODE-POSTGRES/test/integration/gh-issues/130-tests.js:10:16
    at /home/gurjeet/dev/NODE-POSTGRES/test/test-helper.js:159:16
    at ChildProcess.exithandler (child_process.js:641:7)
    at ChildProcess.EventEmitter.emit (events.js:98:17)
    at maybeClose (child_process.js:743:16)
    at Process.ChildProcess._handle.onexit (child_process.js:810:5)

 AssertionError: expected Error: Command failed: psql: FATAL:  role
"gurjeet" does not exist
 to be null
    at Function.assert.isNull
(/home/gurjeet/dev/NODE-POSTGRES/test/test-helper.js:170:10)
    at
/home/gurjeet/dev/NODE-POSTGRES/test/integration/gh-issues/130-tests.js:10:16
    at /home/gurjeet/dev/NODE-POSTGRES/test/test-helper.js:159:16
    at ChildProcess.exithandler (child_process.js:641:7)
    at ChildProcess.EventEmitter.emit (events.js:98:17)
    at maybeClose (child_process.js:743:16)
    at Process.ChildProcess._handle.onexit (child_process.js:810:5)

xargs: node: exited with status 255; aborting
make: *** [test-integration] Error 124
npm ERR! Test failed.  See above for more details.
npm ERR! not ok code 0

And:

api-tests.js (binary)..........
heroku-pgpass-tests.js (binary).Message: Expected execution of function to
be fired within 5000 milliseconds NaN to change timeout globally)
AssertionError: Expected execution of function to be fired within 5000
milliseconds NaN to change timeout globally)
    at null._onTimeout
(/home/gurjeet/dev/NODE-POSTGRES/test/test-helper.js:138:12)
    at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)

 AssertionError: Expected execution of function to be fired within 5000
milliseconds NaN to change timeout globally)
    at null._onTimeout
(/home/gurjeet/dev/NODE-POSTGRES/test/test-helper.js:138:12)
    at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)

xargs: node: exited with status 255; aborting
make: *** [test-binary] Error 124
npm ERR! Test failed.  See above for more details.
npm ERR! not ok code 0

On Tue, Jun 17, 2014 at 10:11 AM, Brian C notifications@github.com wrote:

Very nice - thank you sir.

Just as an aside: The tests are a bit of a mess honestly - they were made
mostly in a time before things like mocha or node-tap existed. I have
considered a few times porting them to a nicer test framework than my home
grown makefile rube-goldberg machine, but I can't bring myself to do a
major refactor of the tests at this point. I approached almost the entire
development in a TDD style way and now that they're passing I think it's
too risky to rip them all out and plop them into a more sophisticated
framework. They've also accumulated a lot of small syntax ugliness over the
years as other people have contributed, etc. I also neglect style
guidelines a bit there because "Hey I'm already writing tests - I'm doing
the right thing! I'm gonna relax a bit." So...thanks for the cleanup. [image:
😄]

One problem: one of the tests is now failing. Would you be able to fix
that failing test? Afterwards I'll merge 'em all in.

Reply to this email directly or view it on GitHub
#608 (comment).

Gurjeet Singh http://gurjeet.singh.im/

EDB www.EnterpriseDB.com http://www.enterprisedb.com

Re-add the test for SELECT tag; I was wrong in my assumption.
Postgres generally does not emit a SELECT tag after a SELECT query, but
it does emit that tag after a CREATE TABLE x AS SELECT query.

Example:

postgres=# create table t as select 1;
SELECT 1
@gurjeet

This comment has been minimized.

Show comment Hide comment
@gurjeet

gurjeet Jun 23, 2014

Contributor

Hi I re-added the SELECT tag test based on recent discovery. And incidentally the Travis test run also passed. So I guess this branch is ready for merge.

Contributor

gurjeet commented Jun 23, 2014

Hi I re-added the SELECT tag test based on recent discovery. And incidentally the Travis test run also passed. So I guess this branch is ready for merge.

@gurjeet

This comment has been minimized.

Show comment Hide comment
@gurjeet

gurjeet Jul 2, 2014

Contributor

a gentle bump :)

Contributor

gurjeet commented Jul 2, 2014

a gentle bump :)

@brianc

This comment has been minimized.

Show comment Hide comment
@brianc

brianc Jul 6, 2014

Owner

sorry! been super busy w/ work and traveling - will merge these today. ❤️

Owner

brianc commented Jul 6, 2014

sorry! been super busy w/ work and traveling - will merge these today. ❤️

brianc added a commit that referenced this pull request Jul 6, 2014

Merge pull request #608 from gurjeet/misc_unit_test_improvements
Miscellaneous improvements in unit tests.

@brianc brianc merged commit 732e720 into brianc:master Jul 6, 2014

1 check passed

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