Closed Supereg closed 1 year ago
PSchmiedmayer
commented
1 year ago @Supereg Regarding your usability question: I like the visual message in the UI. We could even add a link to the documentation there, that could be doable. I think that the error is more a runtime error and less a user error but this location is a good way for developers to easily spot any mistake and point them to the right location, I like this approach over a command line log.
codecov[bot]
commented
1 year ago Merging #6 (9bf04e3) into main (8547b11) will increase coverage by
0.27%. The diff coverage is58.34%.
Supereg
commented
1 year ago Thank you for the addition, great addition and helps new users of Spezi Account! 🚀
It would be great if you can add an additional UI test that can verify that behavior in your UI tests to ensure that a user can see the message. My suggestion:
- We could define if account services should be added or not using a feature flag (like to do this in the template app) and then either add or not add some account services.
- We can then pass this feature flag in the UI test (like we do it in the template app) and then investigate if the warning message appears.
What do you think?
Sounds good. Implemented exactly that. Thanks for the hints on how to approach this 👍
Supereg
commented
1 year ago @Supereg Regarding your usability question: I like the visual message in the UI. We could even add a link to the documentation there, that could be doable. I think that the error is more a runtime error and less a user error but this location is a good way for developers to easily spot any mistake and point them to the right location, I like this approach over a command line log.
Yes that is basically where my original motivation was coming from when I tried to understand the architecture of the app but couldn't locate the AccountServices buttons in the SwiftUI Previews. I added a button to the UI that opens the respective SpeziAccount documentation page.
Supereg
commented
1 year ago Okay, I tried to debug this for 2 hours now. With the changes made in this PR the App freezes when clicking any of the AccountServiceButtons (e.g. open UITest app -> click "Login" -> click "Username and Password" -> freeze). At this point the app will continuously eat memory. When switching to the main branch this doesn't happen.
EDIT: for some reasons this was related to just having the @Environment(\.openURL) private var openURL property wrapper defined in the AccountServicesView. Idk why this would case such behavior. Though, I noticed that the AccountServicesView preview won't ever load in Xcode and times out, maybe there is still another issue?
Supereg
commented
1 year ago Any ideas why the code coverage still fails with 0%? Seems off by a lot 😬
PSchmiedmayer
commented
1 year ago @Supereg I did resolve the code coverage issue using some additional flags. The tool now correctly reports the code coverage report.
Unfortunately we have the issue that the if #DEBUG statement seems to be ignored in the Swift Package which results in changes in the preview being reported in the code coverage. I was not able to find a good workaround for that and will track that issue in a separate issue in the main Swift Package Template repo.
PSchmiedmayer
commented
1 year ago @Supereg The one test seems to fail due to an issue with our GitHub runner, I will check this in the office tomorrow morning: https://github.com/StanfordSpezi/SpeziAccount/actions/runs/5298702381/jobs/9601398047?pr=6
The UI test seems to fail due to an iOS 17 problem on the runners: https://github.com/StanfordSpezi/SpeziAccount/actions/runs/5298702381/jobs/9601398175?pr=6
Tracked the issue in this GitHub issue: https://github.com/StanfordBDHG/XCTestExtensions/issues/11
This PR should address the issue, will merge it once I have full iOS 17 compatibility ready: https://github.com/StanfordBDHG/XCTestExtensions/pull/12
Supereg
commented
1 year ago I double checked, and the CI uses the updated version of XCTestExtensions. UI Tests are passing locally. UI Tests are failing in the CI with this error getting spammed into the logs:
2023-06-22 08:55:11.779 xcodebuild[13749:200357] [MT] IDEFileReferenceDebug: [Load] <IDEFileReference, 0x600007a1fe00: name:SpeziAccount.docc path:absolute:/Users/githubrunner/actions-runner/_work/SpeziAccount/SpeziAccount/Sources/SpeziAccount/SpeziAccount.docc> Failed to load container at path: /Users/githubrunner/actions-runner/_work/SpeziAccount/SpeziAccount/Sources/SpeziAccount/SpeziAccount.docc, Error: Error Domain=com.apple.dt.IDEContainerErrorDomain Code=6 "Cannot open "SpeziAccount.docc" as a "Folder" because it is already open as a "Swift User Managed Package Folder"." UserInfo={NSLocalizedDescription=Cannot open "SpeziAccount.docc" as a "Folder" because it is already open as a "Swift User Managed Package Folder".}
Just to track it here as well.
Show a message when no account services are set up
:recycle: Current situation & Problem
Currently, when there are no
AccountServices set up theAcountServicesViewjust displays nothing instead of the button. Especially for developers new to the framework ecosystem, they might be unfamiliar that AccountServices are set up globally and that the respective Signup/Login buttons are place automatically.:bulb: Proposed solution
This PR tries to avoid this problem by giving a visual hint on why no
AccountServiceButtons are shown.:gear: Release Notes
AccountServices are set up.:heavy_plus_sign: Additional Information
@PSchmiedmayer We might consider an alternative where we don't display the diagnostic message but only print the message to the app logger. Are there any guidelines on how to deal with (configuration) issues like these in the Spezi framework ecosystem?
Additionally, the PR fixes some missing translations and misspelled translations in previews.
Related PRs
--
Testing
This case is currently not covered in the UI tests.
Reviewer Nudging
All the primary changes happen in the
AccountServicesView.Code of Conduct & Contributing Guidelines
By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:
Attachments