firebase / firebase-functions-test

MIT License
232 stars 48 forks source link

makeDataSnapshot signature not accepting null values #48

Closed tjokimie closed 3 years ago

tjokimie commented 4 years ago

firebase-functions-test makeDataSnapshot signature is not accepting null values.

This makes it impossible to test code using change.after.exists() without using @ts-ignore, for example:

const foo = functions.database.ref('/foo').onWrite(async (change, context) => {
  if (!change.after.exists()) {
    // skip
    return
  }
  // actual logic
})

In test you would want to use the following code but it throws src/responses/__tests__/index.test.ts:143:54 - error TS2345: Argument of type 'null' is not assignable to parameter of type 'string | number | boolean | object | any[]'.

const afterSnap = test.database.makeDataSnapshot(null, 'foo')
const change = test.makeChange(null, afterSnap)

firebase-functions DataSnapshot constructor still accepts null values as it has any type.

Related signatures:

makeDataSnapshot signature: https://github.com/firebase/firebase-functions-test/blob/8ef3a117d19fc0729e151348733a63fab0bbe05d/src/providers/database.ts#L31

DataSnapshot constructor in firebase-functions: https://github.com/firebase/firebase-functions/blob/cc663231245256c444e54211911c25fbda0db3d3/src/providers/database.ts#L336

Proposed fix:

The makeDataSnapshot signature should be changed either so that it would also allow null values or unify it with DataSnapshot constructor by allowing any values.

laurenzlong commented 4 years ago

Implemented in https://github.com/firebase/firebase-functions-test/pull/53

rfitz123 commented 4 years ago

I'm having an issue with makeDocumentSnapshot, used like so:

const snapshot = test.firestore.makeDocumentSnapshot({owner: 'testUserA', text: 'response from test user a'}, 'users/testUserA/questions/testQuestion/responses/testResponse');

That line gives me the error

(node:28967) UnhandledPromiseRejectionWarning: SyntaxError: Unexpected token u in JSON at position 0
    at JSON.parse (<anonymous>)
    at App.getApp (/Users/riley/Work/Agape/agape-staging-api/functions/node_modules/firebase-functions-test/lib/app.js:40:65)
    at Object.makeDocumentSnapshot (/Users/riley/Work/Agape/agape-staging-api/functions/node_modules/firebase-functions-test/lib/providers/firestore.js:44:71)
    at Context.<anonymous> (/Users/riley/Work/Agape/agape-staging-api/functions/test/notification.test.js:100:37)
    at callFnAsync (/Users/riley/Work/Agape/agape-staging-api/functions/node_modules/mocha/lib/runnable.js:423:21)
    at Test.Runnable.run (/Users/riley/Work/Agape/agape-staging-api/functions/node_modules/mocha/lib/runnable.js:363:7)
    at Runner.runTest (/Users/riley/Work/Agape/agape-staging-api/functions/node_modules/mocha/lib/runner.js:546:10)
    at /Users/riley/Work/Agape/agape-staging-api/functions/node_modules/mocha/lib/runner.js:672:12
    at next (/Users/riley/Work/Agape/agape-staging-api/functions/node_modules/mocha/lib/runner.js:455:14)
    at /Users/riley/Work/Agape/agape-staging-api/functions/node_modules/mocha/lib/runner.js:465:7
    at next (/Users/riley/Work/Agape/agape-staging-api/functions/node_modules/mocha/lib/runner.js:367:14)
    at Immediate.<anonymous> (/Users/riley/Work/Agape/agape-staging-api/functions/node_modules/mocha/lib/runner.js:433:5)
    at processImmediate (internal/timers.js:456:21)
    at process.topLevelDomainCallback (domain.js:137:15)

It seems to follow the documentation here.

firebase-functions version is 3.3.0 firebase-functions-test version is 0.1.7

I originally posted this on StackOverflow here.

vractal commented 4 years ago

Did you ever find out what it was @rfitz123 ?

vractal commented 4 years ago

It seems that something is cleaning up the env vars in the wrong time, (or I am missing something), specifically FIREBASE_CONFIG, which is used in /node_modules/firebase-functions-test/lib/app.js:40.. Running my troubled test by itself, everything goes well, but when I run all tests, I get same error as above ( SyntaxError: Unexpected token u in JSON at position 0) . After commenting out all test.cleanup() from all tests (not just inside the file), then I can run all tests without a problem.

vractal commented 4 years ago

Should I open a new issue for this? Doesnt seem really related to the initial issue @laurenzlong

inlined commented 3 years ago

Yes, if you are still encountering this issue, please open another bug. The issue you are (were?) facing has nothing to do with supporting null values.

MorenoMdz commented 2 years ago

Should I open a new issue for this? Doesnt seem really related to the initial issue @laurenzlong

Hey @vractal have you opened that new issue after all? We are facing the same issue here