-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Make native-messaging-test-runner use desktop_proxy #11923
Draft
dani-garcia
wants to merge
7
commits into
main
Choose a base branch
from
ps/make-native-test-runner-use-desktop-proxy
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
No New Or Fixed Issues Found |
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #11923 +/- ##
=======================================
Coverage 33.53% 33.53%
=======================================
Files 2886 2886
Lines 90152 90152
Branches 17135 17135
=======================================
Hits 30235 30235
Misses 57527 57527
Partials 2390 2390 ☔ View full report in Codecov by Sentry. |
# Conflicts: # package-lock.json # package.json
# Conflicts: # apps/desktop/native-messaging-test-runner/package-lock.json # apps/desktop/native-messaging-test-runner/package.json # package-lock.json
# Conflicts: # package-lock.json
# Conflicts: # apps/desktop/native-messaging-test-runner/package-lock.json # apps/desktop/native-messaging-test-runner/package.json # package-lock.json
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🎟️ Tracking
📔 Objective
While debugging some issues around the DDG integration I've changed the way the test runner connects to the IPC socket and made it use the
desktop_proxy
binary rather than a direct IPC connection. This matches better with what the browsers actually do, so I think this should be a better way to test compatibility.Coincidentally, the test runner hasn't been working correctly since the IPC changes, as the IPC socket path has changed to be located in a sandboxed location rather than the user
tmp
dir. The use ofdesktop_proxy
also fixes that, as it's using the correct directory.I've only tested this lightly and for the purposes of debugging DDG, so it might need some extra testing.
Also, at the moment I've hardcoded the path to
desktop_proxy
to be inside/Applications/Bitwarden.app
. This will make it work against AppStore and TestFlight builds, and DMG builds if the application is installed into theApplications
folder.I've also included a commented out option to use a local debug build of it. When testing against a prod application we want to use the prod
desktop_proxy
, while when testing against a debug build ornpm run electron
we want to use the debug build, as the IPC won't be in a sandboxed path in this case. I'm not sure how this test runner is used by others in practice, so we might want to let this path be configurable.📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:
) or similar for great changes:memo:
) or ℹ️ (:information_source:
) for notes or general info:question:
) for questions:thinking:
) or 💭 (:thought_balloon:
) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:
) for suggestions / improvements:x:
) or:warning:
) for more significant problems or concerns needing attention:seedling:
) or ♻️ (:recycle:
) for future improvements or indications of technical debt:pick:
) for minor or nitpick changes