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

GPII-1897: Isolate buildup and teardown to allow for persistence between runs. #12

Merged
merged 53 commits into from
Dec 1, 2016

Conversation

the-t-in-rtf
Copy link
Collaborator

See GPII-1897 for details.

@the-t-in-rtf
Copy link
Collaborator Author

the-t-in-rtf commented Aug 15, 2016

Even with my best efforts, I cannot seem to get the in-memory memdown-backed grade to consistently clear itself out. I get constant errors like:

{ [conflict: Document update conflict]
  status: 409,
  name: 'conflict',
  message: 'Document update conflict',
  error: true }

I only get the errors when loading test data, likely these are records with hard-coded _id values, which cannot be overridden (records without would silently result in duplicates). I have a few checks to confirm that no duplicates are created, but obviously not enough, as I can't catch the problem behavior even across a wider range of test runs.

We should look at this as part of this PR. It may be that we can mitigate this by removing in-memory storage as an option for express-pouchdb, and only allowing in-memory databases on the browser side. Now that the filesystem-backed grade has been better fleshed out, I'm happy only using that in my own work.

@the-t-in-rtf
Copy link
Collaborator Author

The new gpii.pouch grade is now integrated into gpii.pouch.express. This should be ready for further review, and for Cindy and Simon to examine.

dbComponentOptions.dbPaths = expandedDef.data;
}

var dbComponent = fluid.component(dbComponentOptions);
Copy link
Member

Choose a reason for hiding this comment

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

This must never be done - the fact that it still works is just the blindest accident and will be cleaned up in the next framework update. Always use initDependent (if you must have a programmatic API) or preferably the Nexus methods such as fluid.construct

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The programmatic API was used to support the previous databases option rather than forcing everyone to define subcomponents for each database. I will try the Nexus methods here and we can review later.

Copy link
Collaborator Author

@the-t-in-rtf the-t-in-rtf Aug 30, 2016

Choose a reason for hiding this comment

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

Done, I migrated to using fluid.construct.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looking at this in light of other comments, dbOptions can easily become the basis of the options set for the "array of dynamic components" we've been discussing.

@the-t-in-rtf
Copy link
Collaborator Author

This needs to be merged after gpii-express, so that we can move to a published version.


| Option | Type | Description |
| -------------------- | ---------- | ----------- |
| `baseDir` (required) | `{Number}` | A full or package-relative path to the base directory in which all database content, configuration files, and logs will be stored. |
Copy link
Member

Choose a reason for hiding this comment

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

Type of {Number} doesn't seem right here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

@the-t-in-rtf
Copy link
Collaborator Author

@cindyli, I have completed the refactor we discussed in our last meeting. We now use the previous "destroy" method to cleanup database instances. Please confirm if this works for you.

@amb26
Copy link
Member

amb26 commented Nov 22, 2016

Note that the strategy for sequencing test runs in package.json fails on Windows:
"test": "npm run test:browser; npm run test:node",

@the-t-in-rtf
Copy link
Collaborator Author

the-t-in-rtf commented Nov 23, 2016

Thanks, @amb26. Based on our frequent use of && across platforms, I foolishly assumed a semicolon would work as well. I have updated the definition to use && instead.

@the-t-in-rtf
Copy link
Collaborator Author

To summarize, after much effort the project is in a usable state at the moment. As mentioned above, I have created issues for the remaining work, which now includes tracking down the underlying source of the 409 errors:

  1. GPII-2162 (document conflicts)
  2. GPII-1995 (migrating away from event wrappers)
  3. GPII-2163 (using dynamic components instead of the current databases option in gpii.pouch.express)

@cindyli and @amb26, I hope that a) I have summarized the work remaining correctly and b) we can agree to merge what we have.


var PouchDB = require("pouchdb");

var path = require("path"); // eslint-disable-line no-unused-vars
Copy link
Member

Choose a reason for hiding this comment

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

With respect to the discussion below about inclusion of "path", this require is ineffective. All it does is load path into your global object, not into Infusion's.
The safe way of ensuring that "path" is available for global resolution is to write
fluid.require("path", require, "path");
http://docs.fluidproject.org/infusion/development/NodeAPI.html#fluid-require-modulename-foreignrequire-namespace-

Copy link
Collaborator Author

@the-t-in-rtf the-t-in-rtf Dec 1, 2016

Choose a reason for hiding this comment

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

Thanks, and fixed. Given that I had to tiptoe around the ESLint "unused variable" warning, it should be easy enough to remember when this is relevant in the future.

@amb26 amb26 merged commit 48c4807 into fluid-project:master Dec 1, 2016
@the-t-in-rtf the-t-in-rtf deleted the GPII-1897 branch April 26, 2017 14:52
@the-t-in-rtf the-t-in-rtf restored the GPII-1897 branch July 26, 2017 13:35
@the-t-in-rtf the-t-in-rtf deleted the GPII-1897 branch November 22, 2017 09:03
@the-t-in-rtf the-t-in-rtf restored the GPII-1897 branch January 25, 2018 11:20
@the-t-in-rtf the-t-in-rtf deleted the GPII-1897 branch February 6, 2018 09:23
@the-t-in-rtf the-t-in-rtf restored the GPII-1897 branch May 22, 2018 18:19
@the-t-in-rtf the-t-in-rtf deleted the GPII-1897 branch May 22, 2018 18:20
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.

3 participants