Add testing module #514
Add testing module #514
Conversation
| credential: new FakeCredentials(), | ||
| databaseURL: DBURL + '?ns=' + options.databaseName | ||
| }, | ||
| 'app-' + (new Date().getTime() + Math.random()) |
There was a problem hiding this comment.
I think this Math.random() is actually increasing the chance of a name collision. Can we just use the timestamp or an autoincrement id?
| databaseURL: DBURL + '?ns=' + options.databaseName, | ||
| databaseAuthVariableOverride: options.auth || null | ||
| }, | ||
| 'app-' + (new Date().getTime() + Math.random()) |
There was a problem hiding this comment.
maybe call this admin-app-{}?
There was a problem hiding this comment.
This looks pretty good, a couple of things to address. Small things are in the comments.
I did notice, that you have the package-lock.json file in this package. I'm a little worried about that as it means you used NPM, instead of yarn to install/manage dependencies.
If you could delete that, clean the git tree (git clean -xdf) and then do a reinstall of your dependencies from the root using yarn that'll give me some piece of mind. If there are any yarn.lock changes from that, those will need to be committed.
| @@ -0,0 +1,39 @@ | |||
| { | |||
| "name": "@firebase/testing", | |||
| "version": "0.1.0", | |||
There was a problem hiding this comment.
Lets make sure we have the private value set to true for this module. I don't think we want to publish this quite yet?
packages/testing/package.json
Outdated
| "module": "dist/esm/index.js", | ||
| "scripts": { | ||
| "dev": "gulp dev", | ||
| "test": "run-p test:node", |
There was a problem hiding this comment.
If you don't have any browser tests you can omit the npm-run-all dependency, and replace this test script with the one in test:node
|
Currently, there is no line in the Want to add the team working on this as a line there? Something like: |
The goal of this PR is to add a new module
@firebase/testingthat is a relatively stand alone module that can be included to test js apps locally.