-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Expose DatabaseAdapter to simplify application tests #1121
Conversation
…estutils # Conflicts: # src/index.js
…estutils # Conflicts: # src/index.js
Are the checks failing due to this change or what is being addressed with #1122? |
nope: see here
|
…estutils # Conflicts: # src/index.js
@steven-supersolid updated the pull request. |
Current coverage is
|
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 |
Now that I've used the code, I'm a bit nervous even having the
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. |
Thats true, but it's also digging around in an internal interface, which might change at any time. What about having the |
@@ -49,6 +49,15 @@ function clearDatabaseSettings() { | |||
appDatabaseOptions = {}; | |||
} | |||
|
|||
//Used by tests | |||
function clearData() { |
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.
we could wrap here in:
var clearData = function() {
throw '';
}
if (process.env.TESTING) {
clearData = function() {}
}
…DataPermanently from DatabaseAdapter. Update helper
@steven-supersolid updated the pull request. |
@@ -49,6 +49,18 @@ function clearDatabaseSettings() { | |||
appDatabaseOptions = {}; | |||
} | |||
|
|||
//Used by tests | |||
function destroyAllDataPermanently() { | |||
if (process.env.TESTING) { |
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 wrapped this a little differently, can change if there is any advantage to the previously suggested way of doing this?
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.
that's ok too
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. |
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. |
I'm okay with this. |
…tions from other modules, only in test environment
@steven-supersolid updated the pull request. |
…estutils # Conflicts: # src/index.js
@steven-supersolid updated the pull request. |
Not sure why one of the builds failed. If the code looks good I'll try pushing again |
Looks like it's passing. I'm pretty happy with how this turned out, thanks @steven-supersolid! |
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()