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

Expose DatabaseAdapter to simplify application tests #1121

Merged

Conversation

steven-supersolid
Copy link
Contributor

When running tests in an application that uses parse-server it would be convenient to clear the database in a similar fashion to in helper.js.

Also moved the clearData util function to avoid having that in helper.js and application helper.js, so this now behaves like clearDatabaseSettings()

@steven-supersolid
Copy link
Contributor Author

Are the checks failing due to this change or what is being addressed with #1122?

@flovilmart
Copy link
Contributor

nope:

see here

ReferenceError: ParseServer is not defined
    at new runServer (/home/travis/build/ParsePlatform/parse-server/src/index.js:9:21985)
    at Object.<anonymous> (/home/travis/build/ParsePlatform/parse-server/spec/helper.js:46:11)
    at Module._compile (module.js:409:26)
    at Object.Module._extensions..js (module.js:416:10)
    at Object.Module._extensions.(anonymous function) [as .js] (/home/travis/build/ParsePlatform/parse-server/node_modules/babel-istanbul/lib/hook.js:109:37)
    at Module.load (module.js:343:32)
    at Function.Module._load (module.js:300:12)
    at Module.require (module.js:353:17)
    at require (internal/module.js:12:17)
    at /home/travis/build/ParsePlatform/parse-server/node_modules/jasmine/lib/jasmine.js:77:5

@facebook-github-bot
Copy link

@steven-supersolid updated the pull request.

@codecov-io
Copy link

Current coverage is 92.78%

Merging #1121 into master will decrease coverage by -0.02% as of f8bfb6c

@@            master   #1121   diff @@
======================================
  Files           86      87     +1
  Stmts         5460    5475    +15
  Branches      1007    1009     +2
  Methods          0       0       
======================================
+ Hit           5067    5080    +13
  Partial         10      10       
- Missed         383     385     +2

Review entire Coverage Diff as of f8bfb6c

Powered by Codecov. Updated on successful CI builds.

@drew-gross
Copy link
Contributor

I think it's a bit dangerous to do this as it makes the entire database adapter interface part of the public interface, and we are making changes to the database adapter. Rather than exporting the entire database adapter, can you export only the function that deletes everything? Also maybe name it something dangerous sounding, like destroyAllDataPermanently maybe.

@steven-supersolid
Copy link
Contributor Author

Now that I've used the code, I'm a bit nervous even having the destroyAllDataPermanently function in the adapter. The original point of this PR was to expose DatabaseAdapter so my application helper.js could use code similar to parse-server helper.js to clear data between tests. As @flovilmart pointed out, I could also just change my require to:

require('parse-server/lib/DatabaseAdapter');

So perhaps this PR doesn't really add anything apart from a way to accidentally delete all your data. In which case I'm happy to close.

@drew-gross
Copy link
Contributor

Thats true, but it's also digging around in an internal interface, which might change at any time. What about having the destroyAllDataPermanently function do nothing unless a testServer: true parameter is passed to ParseServer or env.PRODUCTION = true or something like that?

@@ -49,6 +49,15 @@ function clearDatabaseSettings() {
appDatabaseOptions = {};
}

//Used by tests
function clearData() {
Copy link
Contributor

Choose a reason for hiding this comment

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

we could wrap here in:

var clearData = function() {
 throw '';
}
if (process.env.TESTING) {
 clearData = function() {}
}

…DataPermanently from DatabaseAdapter. Update helper
@facebook-github-bot
Copy link

@steven-supersolid updated the pull request.

@@ -49,6 +49,18 @@ function clearDatabaseSettings() {
appDatabaseOptions = {};
}

//Used by tests
function destroyAllDataPermanently() {
if (process.env.TESTING) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrapped this a little differently, can change if there is any advantage to the previously suggested way of doing this?

Copy link
Contributor

Choose a reason for hiding this comment

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

that's ok too

@drew-gross
Copy link
Contributor

This looks good to me but because this is introducing a new concept to our public interfaces (A test only interface) I'd like @gfosco and @nlutsenko's opinion before merging.

@steven-supersolid
Copy link
Contributor Author

One possibility is to create a TestUtils class that is exported from index.js that then exposes various test functions. I'm not sure if it is possible but perhaps we could do a conditional export of this class too when process.env.TESTING is set.

@gfosco
Copy link
Contributor

gfosco commented Apr 5, 2016

I'm okay with this.

…tions from other modules, only in test environment
@facebook-github-bot
Copy link

@steven-supersolid updated the pull request.

@facebook-github-bot
Copy link

@steven-supersolid updated the pull request.

@steven-supersolid
Copy link
Contributor Author

Not sure why one of the builds failed. If the code looks good I'll try pushing again

@drew-gross
Copy link
Contributor

Looks like it's passing. I'm pretty happy with how this turned out, thanks @steven-supersolid!

@drew-gross drew-gross merged commit 30197a7 into parse-community:master Apr 8, 2016
@steven-supersolid steven-supersolid deleted the steven.database.testutils branch August 4, 2016 22:44
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.

6 participants