Hotfix analytics 5.8.8+1
#265
Conversation
* Use new private `_sendError` for error handler * Prep publishing * Update README to include diff
| // Skipping this test since the hotfix affects how many records are written | ||
| // to the log file | ||
| test(skip: true, 'only sends one event for FormatException', () { |
There was a problem hiding this comment.
@christopherfujino @andrewkolos thoughts on skipping the breaking tests for this hotfix.. the patch in this hotfix breaks some tests that are checking the log file for written records.
It's either we skip or i change/remove these tests.
We can also leave the tests alone since merging into a separate branch from main for dart-lang/tools doesn't require us to have tests passing
There was a problem hiding this comment.
Why does this change break the tests? I think it would be generally an undesirable thing to skip tests on a hotfix, but if there are mitigating circumstances and we're 100% confident the code is right and the failure is invalid, it would be reasonable to skip for the hotfix.
But if that is the case we should document why we believe it's safe to skip and why it was not feasible to fix the tests.
There was a problem hiding this comment.
The reason these tests are breaking is because we are no longer saving the error event we send to the locally persisted file. And the reason I'm not saving the error event locally is because the schema for the user property map has changed which will always cause an error when we attempt to parse records out of the file.
Having a record from the log file not get parsed correctly does not cause an error though but it will cause tests to break.
Additionally, the log file is only parsed when we call the Analytics.logFileStats method which is only being used when we have contextual surveys launched, which we currently don't at the moment
There was a problem hiding this comment.
I can also fix these tests so they are passing but i didnt know what the best practice was here for a hotfix.. since the test will be changed again back to the default for future versions
There was a problem hiding this comment.
I can also fix these tests so they are passing but i didnt know what the best practice was here for a hotfix
We sync'ed offline, but for posterity the best practice with hotfixes is to have passing tests that correct validate the behavior in the fix--in other words, we should update tests for hotfix changes (unless there are some mitigating circumstances where this isn't feasible).
There was a problem hiding this comment.
Tests have been updated, let me know if this looks good for releasing
Proactive fix for:
Contribution guidelines:
dart format.Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.