Skip to content

Update the test:setup script to configure firestore settings #240

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

Merged
merged 4 commits into from
Oct 23, 2017

Conversation

jshcrowthe
Copy link
Contributor

@jshcrowthe jshcrowthe commented Oct 18, 2017

It seems some styling things got missed by another PR (i.e. #232)

This just augments the test:setup script to properly configure firestore

Copy link

@gauntface gauntface left a comment

Choose a reason for hiding this comment

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

Changes LGTM. Did I miss a linting or prettier or ... step in my previous PR?

Also request the test setup - is there any way to test that it's setup correctly and tell the developer to change if it's not?

i.e. write to firestore, catch the error and give a useful error message on what to do?

@gauntface
Copy link

Also looks like the messaging integration tests are failling, has this been an ongoing issue? they passed for my previous PR (I think)

@jshcrowthe
Copy link
Contributor Author

@gauntface I've seen your integration tests time out from time to time. It is really inconsistent. We can try bumping the timeouts?

@gauntface
Copy link

timeouts and retries should make it more stable. Happy to take a look in a seperate PR unless you want to try in this one.

@jshcrowthe
Copy link
Contributor Author

Lets do it in a separate one.

@jshcrowthe jshcrowthe merged commit 52a4cdf into master Oct 23, 2017
@jshcrowthe jshcrowthe deleted the firestore-config branch October 23, 2017 15:07
@firebase firebase locked and limited conversation to collaborators Oct 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants