-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Fixed issue that prevented Postgres Tests from passing locally and on any port other than 5432 in travis #6531
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
Changes from 109 commits
841768c
33da65e
43b714d
8010633
cc2506a
05181a7
1ff769d
fba1b83
edf60e8
c884e5d
e553200
c7b4411
063dd2f
d357a47
b048966
299b91d
6adf026
87e37a1
c20a82e
76a29d0
b90abc4
5d4019d
028daee
387258c
dc3afa6
06045a5
ebcbbb0
72f72a1
2047e2e
fa10637
0bdc4f4
d60203d
a7c964b
d3e795a
b0e224f
98d83e9
ddd6f4a
efa17ff
6a98945
a3f72b8
5999aff
579124c
38a2b9e
8b0f875
ac3be67
0052963
a375604
8d26cbc
dfcbe0d
a5aa417
cc91dc4
0e33072
773ca67
fe0077c
559e0a7
2ceff98
57a81cb
7776457
59a3e8a
3a099c1
fc2006d
4ffee37
8d17b04
84b4a64
839d3e6
c1173cb
648ea74
3842083
6a6c2d4
fca6973
cc0f9c7
bcf2116
530bc9d
b4c6ee0
b8ee28e
bdac2f4
e90eeae
4192af3
48d1461
5265115
4cd60df
183e273
5457858
5a175d5
2455281
36d590a
ee57800
8300f20
dc6e595
59c9031
8e27b46
268c08a
f93620f
c1dde02
b269e6b
bd5a1f8
160f5a5
2eb7987
f635706
fff3f0d
f03310e
020080e
79c7949
c27c304
bb86193
4a8650e
a70e1f9
9073081
a03d511
fc73f17
861f048
8d40f0c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,25 +1,14 @@ | ||
'use strict'; | ||
|
||
const MongoStorageAdapter = require('../lib/Adapters/Storage/Mongo/MongoStorageAdapter') | ||
.default; | ||
const mongoURI = | ||
'mongodb://localhost:27017/parseServerMongoAdapterTestDatabase'; | ||
const PostgresStorageAdapter = require('../lib/Adapters/Storage/Postgres/PostgresStorageAdapter') | ||
.default; | ||
const postgresURI = | ||
'postgres://localhost:5432/parse_server_postgres_adapter_test_database'; | ||
const Config = require('../lib/Config'); | ||
const Parse = require('parse/node'); | ||
const request = require('../lib/request'); | ||
let databaseAdapter; | ||
|
||
const fullTextHelper = () => { | ||
if (process.env.PARSE_SERVER_TEST_DB === 'postgres') { | ||
if (!databaseAdapter) { | ||
databaseAdapter = new PostgresStorageAdapter({ uri: postgresURI }); | ||
} | ||
} else { | ||
databaseAdapter = new MongoStorageAdapter({ uri: mongoURI }); | ||
} | ||
const config = Config.get('test'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is necessary. Have you read the Contributor Guide Does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dplewis this test fails locally on my system using that command if I don’t have Postgres running on localhost using the standard port, 5432. This fix makes it work on any address/port Postgres is binded to by using the already defined PARSE_SERVER_TEST_DATABASE_URI environment variable. In the test cases current form, PARSE_SERVER_TEST_DATABASE_URI doesn’t work as You can replicate the failure by dowloading Postgres via docker and binding to a different port. I suspect this is why the Travis file has to change the Postgres config to 5432 as the newly installed Postgres installations use 5433 and the PARSE_SERVER_TEST_DATABASE_URI doesn’t work properly Edit: because of ParseQuery.FullTextSearch.spec.js and PostgresInitOptions.spec.js (the only two tests that fail) has the URI for postgres/mongo hardcoded instead of leveraging the config file like other tests (see my initial fix). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To my last point, after this pull request is reconciled, I plan on creating another one which edits the travis file and scripts to not have to stop/start postgres, nor have to remove older versions of postgres that are running on port 5432. Using PARSE_SERVER_TEST_DATABASE_URI should allow us to test on port 5433 directly after install of postgres without changing the image postgres config files (hopefully). I'm assuming this will speed up the postgres builds along with keeping the fix of the old postgres build issues. If you think it's relevant enough, I can make the changes here if you think they are related enough and we can see how they go There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In addition, PARSE_SERVER_TEST_DATABASE_URI isn't mentioned in the Contributors Guide, but it is in the spec/helper.js and spec/ParseServer.js files. I can add this in as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lastly, if anyone attempts to run a local test using the official Postgres (which PostGIS docker image uses), POSTGRES_PASSWORD has to be set or else the image won't build. This means that all local postgres tests using docker will require the use if PARSE_SERVER_TEST_DATABASE_URI as the default postgres URI set in the spec won't work. See my example below:
Note that I didn't use the mdillion/postgis image discussed in the Contributors Guide as it hasn't been updated in over a year. |
||
databaseAdapter = config.database.adapter; | ||
|
||
const subjects = [ | ||
'coffee', | ||
'Coffee Shopping', | ||
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change the run test command to include
npm run testonly
so that it doesn't start the mongodb-runnerPARSE_SERVER_TEST_DB=postgres PARSE_SERVER_TEST_DATABASE_URI=postgres://postgres:password@localhost:5432/parse_server_postgres_adapter_test_database npm run testonly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cbaker6 Can you run the testonly locally? Making sure you don't have mongo running locally? I have 6 failing tests that require mongodb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can try, I've always ran "npm test" because of the older versions of the docs
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testonly
was added because we didn't want to keep starting the mongodb-runner. It makes testing locally faster.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future can you delete your fork, re create it and work off of branches instead of your master branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dplewis thanks, I didn't know this. I will try it next time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though we squash commits. As you can see there are 60+ commits that carried over from previous PRs that you have done. A rule of thumb is to keep the master branch on your fork up to date and branch off of that before every PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what i do is create a 'feature' branch that I put all my commits into and I periodically merge master into it.
making tons of small commits is a good practice imho (not disagreeing with dplewis, just adding my$.02).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a develop branch that I use similar concept.