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

feat(crashlytics): Firbase web modular V9 API #7283

Merged
merged 3 commits into from
Sep 4, 2023

Conversation

exaby73
Copy link
Contributor

@exaby73 exaby73 commented Aug 8, 2023

Description

Related issues

Release Summary

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • My change includes tests;
    • e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Test Plan


Think react-native-firebase is great? Please consider supporting the project with any of the below:

@vercel
Copy link

vercel bot commented Aug 8, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-native-firebase ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 4, 2023 7:17pm
react-native-firebase-next ❌ Failed (Inspect) Sep 4, 2023 7:17pm

Copy link
Member

@russellwheatley russellwheatley left a comment

Choose a reason for hiding this comment

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

Nice work. Much better having no duplicate tests running. Like that we have a declaration file for the modular API. Only one change from me 👍

iOS CI seems flaky and unrelated. Also - code coverage CI is picking up code that is covered, not sure what to do about that but ignore?

packages/crashlytics/lib/modular/index.js Outdated Show resolved Hide resolved
@exaby73
Copy link
Contributor Author

exaby73 commented Aug 16, 2023

Code coverage has been having trouble with the modular APIs. Most if not all of the code is tested, but some functions that can't be tested (and that weren't tested in the V8 APIs either) cause the coverage to go down. Not sure what to do about those. I don't feel writing noop tests to make coverage happy is a good idea so I left it so.

As for the flakiness, it has always been flaky but now since we are essentially testing the same flaky code twice, just wrapped in modular functions, the probability of flakiness showing itself is higher.

@codecov
Copy link

codecov bot commented Aug 25, 2023

Codecov Report

Merging #7283 (35094f4) into main (794e081) will decrease coverage by 0.02%.
The diff coverage is 42.31%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7283      +/-   ##
============================================
- Coverage     53.79%   53.77%   -0.02%     
  Complexity      735      735              
============================================
  Files           231      232       +1     
  Lines         11633    11659      +26     
  Branches       1867     1867              
============================================
+ Hits           6257     6268      +11     
- Misses         5029     5044      +15     
  Partials        347      347              

russellwheatley
russellwheatley previously approved these changes Sep 4, 2023
@mikehardy
Copy link
Collaborator

Looking through this now - as a general comment though - please don't mix feature/fix changes with dependency changes, and please don't do merge commits. Much better to rebase off main and repush. I'll do so and drop the various deps + merge commit stuff so the PR is easier to review

CI should not be flaky either - that's something that should be a specific comment as needed. It does flake but I try to squash the flakes as soon as possible - that was actually a focus end of last week and I think things are purring along pretty well now

tests/e2e/.mocharc.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

rebased this and repushed to clear out the files that were inadvertently commited + reverted and to remove the merge, looks clean now

just one note on doc typing

mikehardy
mikehardy previously approved these changes Sep 4, 2023
Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

still investigating the test coverage notes saying there is no coverage when it appears there should be but types are fixed with the typedef suggestion, and I pushed that.

If e2e CI run gets the coverage I expect this will merge immediately, otherwise I'll keep investigating

packages/crashlytics/lib/modular/index.js Outdated Show resolved Hide resolved
packages/crashlytics/lib/index.d.ts Show resolved Hide resolved
@mikehardy mikehardy removed the Workflow: Waiting for User Response Blocked waiting for user response. label Sep 4, 2023
@mikehardy
Copy link
Collaborator

Crashlytics modular methods are not being covered in CI coverage reports

Need to investigate why not, but don't want to block the PR so this is a separate issue

@mikehardy mikehardy merged commit 57f7327 into main Sep 4, 2023
14 of 16 checks passed
@mikehardy mikehardy deleted the feat/crashlytics-modular branch September 4, 2023 22:04
@mikehardy mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Sep 4, 2023
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.

3 participants