-
Notifications
You must be signed in to change notification settings - Fork 265
fix(backup): return error if there is no backup path set #7155
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
Jenkins BuildsClick to see older builds (36)
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #7155 +/- ##
============================================
+ Coverage 34.92% 59.86% +24.93%
============================================
Files 798 813 +15
Lines 111349 113425 +2076
============================================
+ Hits 38891 67900 +29009
+ Misses 67561 38653 -28908
- Partials 4897 6872 +1975
Flags with carried forward coverage won't be shown. Click here to find out more.
|
d741bd5 to
13c938a
Compare
13c938a to
f2898bc
Compare
|
@jrainville looking at the coverage, |
It's not invoked in integration tests, but it should be in Functional tests, as that is how we get the path to the file. Do you know if the coverage works on funcitonal tests too? |
I checked the report and `` doesn't seem to be invoked in functional tests either 🤷 In CI job artifacts, you can find a coverage report (that is uploaded to Codecov) in HTML format:
|
Very weird. I'll double check then, but if that's the case, I don't know where it's getting its file path lol |
|
Seems like there is a bug with the coverage then, because I know for a fact that it's called, because that code block is the one that throws Could it be that since it's an anonymous function, it kinda gets lost by the coverage? @igor-sirotin |
f2898bc to
a5028a7
Compare
👍
Indeed, seems that it's a coverage bug 👀
Seems that it won't help. It's not about the function being anonymous, but the callback being stored as a struct field. I've reimplemented this as an interface, pushed to this PR: 72fe0ce. No matter if this helps or not, we can merge it. |
Needed for status-im/status-app#19377 For Android, we are setting the default path to empty string so that the user can know that they need to modify it. Otherwise, the default path was the datadir and users cannot access it. What this meant is that once the auto-backup was happening, the status-go code worked, but then it failed because of the missing user permission. To fix it, we just throw an error on empty paths in status-go. That way the backup just doesn't happen until the user selects a path.
72fe0ce to
e285de6
Compare

Cherry-pick of #7146
Needed for status-im/status-app#19377
For Android, we are setting the default path to empty string so that the user can know that they need to modify it. Otherwise, the default path was the datadir and users cannot access it.
What this meant is that once the auto-backup was happening, the status-go code worked, but then it failed because of the missing user permission.
To fix it, we just throw an error on empty paths in status-go. That way the backup just doesn't happen until the user selects a path.