-
Notifications
You must be signed in to change notification settings - Fork 625
Update spec tests from changes in the firebase-js-sdk respository #1401
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
Conversation
Coverage ReportAffected SDKs
Test Logs
NotesHTML coverage reports can be produced locally with./gradlew <product>:checkCoverage . Report files are located at <product-build-dir>/reports/jacoco/ .
|
Binary Size ReportAffected SDKs
Test Logs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a question that may be something to address upstream?
} | ||
} | ||
}, | ||
"clientIndex": 0 | ||
"clientIndex": 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the generator isn't generating a stable order among client indexes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look into this and reply back. There must have been a change between now and November 2019 (4 months ago) that affected this as the spec tests have not been regenerated since then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: The culprit is firebase/firebase-js-sdk#2693, discovered by performing a bisection. I'm continuing to investigate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the change in the spec test is expected; however, in the culprit PR the spec tests for Android (and probably iOS) were not updated accordingly. I will create a separate PR to bring the spec tests up-to-date from the PR so that this PR can encompass only a single update to the spec tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've created a PR, firebase/firebase-js-sdk#2843, to produce stable JSON from generate_spec_json.js
. I will hold off merging this PR until that PR is resolved so that a commit with a noisy diff in the spec tests can be limited to one commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: commit 19ab883 was added to this PR with the stable JSON.
The PR firebase/firebase-js-sdk#2843 caused stable JSON to be generated.
Coverage ReportAffected SDKs
Test Logs
NotesHTML coverage reports can be produced locally with./gradlew <product>:checkCoverage . Report files are located at <product-build-dir>/reports/jacoco/ .
|
Binary Size ReportAffected SDKs
Test Logs |
This change picks up changes to the spec tests, especially firebase/firebase-js-sdk#2790, which renamed the "limboDocs" key in the JSON objects to "activeLimboDocs".