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

Make debug logging of typesense imports expicit and configurable #101

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jsasitorn
Copy link
Contributor

Change Summary

Standardize debug logging for typesense inserts and make this configurable in extension settings. For debugging purposes its good to log inserts, but added support to disable this for production environments.

To better support testing, created a node based test environment runner that can setup firebase environment and run the emulator. This approach also helps streamline testing in package.json and reduce boiler template in test cases. This also allows test case to verify server logs.

PR Checklist

…his standardizes logging across both .onWrite and .backfill, and allows disabling

logging for production builds and improved data privacy. Option can be turned off an off in firebase extension config using LOG_TYPESENSE_INSERTS
@jsasitorn
Copy link
Contributor Author

@jasonbosco something i need to do to get tests running? Thanks

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be used in the other tests as well? backfill*.spec.js and indexOnWriteWrite*.spec.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you can use testEnvironment to cover all the .spec.js tests. As a first pass, I'm converting the tests I've explicitly touched for this feature and the ones for my earlier PR.

Only concern could be added cruft of the test environment, so debatable if its good to keep one legacy style test around.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is starting to be difficult to manage and read. We should rename the scripts to be able to identify them from the name itself. Maybe add some pretest scripts while we're at it, to break them into chunks as well. Something like this:

{
  "scripts": {
    "test": "npm run test:core && npm run test:features && npm run test:logging",
    
    "test:core": "npm run test:core:flattened && npm run test:core:unflattened",
    "test:core:flattened": "cross-env DOTENV_CONFIG=extensions/test-params-flatten-nested-true.local.env firebase emulators:exec --only functions,firestore,extensions 'jest --testRegex=\"backfill.spec\"'",
    "test:core:unflattened": "cross-env DOTENV_CONFIG=extensions/test-params-flatten-nested-false.local.env firebase emulators:exec --only functions,firestore,extensions 'jest --testRegex=\"WithoutFlattening\"'",
    // etc
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, this a great follow up! Cleaning up overall test case organization is a bit out of scope for this PR though, which is focused on improving debug logging, and being able to turn off logging of data in production environments.

Also to note, that if you end up migrating more tests to the test environment, you could use jest style selectors, eg:
"test:core:unflattened": "jest --testRegex="WithoutFlattening""

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.

2 participants