Avoid using hardcoded document paths which could cause conflicts when tests are running concurrently (e.g. on Travis). #250
Conversation
… tests are running concurrently (e.g. on Travis).
| return withTestDb(persistence, db => { | ||
| return db.doc('rooms/Eros').set({ | ||
| desc: 'Stuff related to Eros project...', | ||
| return withTestDoc(persistence, docRef => { |
There was a problem hiding this comment.
Probably not worth the effort, but it would be niceish if we picked either docRef or doc and not both :)
There was a problem hiding this comment.
Yeah. If we did that I'd say we should do it in a separate PR... but I don't think it's worth the effort. :-)
| expect(querySnap.size).to.equal(0); | ||
| expect(querySnap.docs.length).to.equal(0); | ||
| }) | ||
| .then(() => unlisten(), () => unlisten()); |
There was a problem hiding this comment.
Doesn't this swallow test failures?
There was a problem hiding this comment.
Sorta'. I think expect() will store any failures out-of-band and the test will still fail, but if we accidentally threw an exception then it would probably get swallowed. I removed the error handling case; if the test fails I don't care if it cleans up after itself. :-)
[to be clear, I didn't touch this code originally; it just got reformatted and github can't diff whitespace]
| @@ -510,13 +510,12 @@ apiDescribe('Database transactions', persistence => { | |||
| }); | |||
|
|
|||
| asyncIt('cannot have a get without mutations', () => { | |||
| return integrationHelpers.withTestDb(persistence, db => { | |||
| const docRef = db.doc('foo/bar'); | |||
There was a problem hiding this comment.
In this case, I would just leave the old code (so you don't have to to docRef.firestore later). You can still use .doc() to ensure randomness.
There was a problem hiding this comment.
Okay. Not sure it makes much difference, but switched back in the spirit of not changing things unnecessarily. :-)
For context: I had a failure this morning that I think may have been caused by this