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

Encapsulate NetP subjects within observers #1436

Merged
merged 8 commits into from
Aug 9, 2023

Conversation

graeme
Copy link
Collaborator

@graeme graeme commented Aug 4, 2023

Task/Issue URL: https://app.asana.com/0/0/1205210093476964/f
BSK PR: duckduckgo/BrowserServicesKit#447
CC:

Description:

For Update Status View to match designs, I planned to rely on the shared Connection*Observer types to receive events from the PacketTunnnelProvider and other NetP types. However, on doing this, I noticed that we’re exposing the Subject type in order to synchronously access the most recent state.

Exposing these types as it means these events can be sent from anywhere and not just the source of the events. Rather than letting dependency on this propagate into iOS too, I decided to do a quick refactor to clean it up.

Additionally, I’ve created the associated mock types which I will also need throughout the iOS: Network Protection MVP TestFlight build for internal testing project. I decided now was the right time to create them as these types have been altered.

Creating the MockTunnelController is included in this BSK release though it seems somewhat independent. This is so that it can be included in the same BSK release as it will also be needed for the parent task.

Steps to test this PR:

Test StatusConnectionObserver

  1. Connect to NetP
  2. Disconnect from NetP
  3. Ensure connection state updates correctly

Test ConnectionServerInfoObserver

  1. Connect to NetP
  2. Change the server location through the debug menu
  3. Check his updates correctly in the UI

Test ConnectionErrorObserver

  1. Connect to NetP
  2. Simulate a tunnel error through the debug menu (Debug -> Network Protection -> Simulate Failure)
  3. Check the status view to see if the error is shown

Test ConnectivityIssueObserver
Preparation

  • Start Network Protection, make sure you're asked to allow notifications.
  • Allow notifications
  • Save this file to your desktop. It's a script to temporarily block UDP traffic.
  • ruleset.txt

Disconnection

  • Using a terminal window type sudo pfctl -ef ~/Desktop/ruleset.txt
  • Wait until NetP reports a problem and make sure a notification is shown for it.
  • Wait a bit more until NetP reports through a notification that the connection was stopped.
  • Once you're done restore UDP traffic by executing: sudo pfctl -d
  • Test 2: Reconnected notification

Reconnection

  • Run the steps from the previous test up to, and including step 5.
  • Once the notification for the connection issues is shown, run this command in a terminal window to restore UDP traffic: sudo pfctl -d
  • After a while make sure NetP reports it successfully restored the connection.

Test ControllerErrorMesssageObserver

  1. Connect to NetP
  2. Simulate a Controller error through the debug menu (Debug -> Network Protection -> Simulate Failure)
  3. Check the status view to see if the error is shown

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

Copy link
Collaborator

@samsymons samsymons left a comment

Choose a reason for hiding this comment

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

Change looks good to me, though there are a couple stray files that I don't believe should be here. Should be nothing blocking this from being approved once those are gone.

@graeme
Copy link
Collaborator Author

graeme commented Aug 8, 2023

@samsymons I’d addresses those issues. I’m going to delay fixing the merge conflicts until I merge the BSK PRs.

Copy link
Collaborator

@samsymons samsymons left a comment

Choose a reason for hiding this comment

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

New changes look good, nice work!

@graeme graeme merged commit 0f8769c into develop Aug 9, 2023
10 checks passed
@graeme graeme deleted the graeme/hide-netp-subjects-within-observers branch August 9, 2023 09:33
samsymons added a commit that referenced this pull request Aug 13, 2023
* develop:
  Update NetP endpoint (#1459)
  Add support for syncing Credentials (#1368)
  Fix dsym upload to S3
  Bump version to 1.51.1 (48)
  Report failed unit tests to Asana (#1457)
  Use MainActor to ensure FutureExtension tests are dispatched on main queue (#1472)
  Move Make /Applications symlink build phase to later (#1470)
  don't show day 7 when day 0 has been dismissed (#1468)
  Update BSK with autofill 8.1.2 (#1471)
  Add job to create a task on asana if tests fails on develop (#1461)
  Disables a flaky test from App Store unit tests (#1467)
  Fix an autoconsent exception on old Safari (#1460)
  fix bug where wrong address bar text is shown when opening DuckPlayer… (#1434)
  Fix FutureExtensionTests (#1450)
  Add pixel to determine Bitwarden use on Mac (#1412)
  Encapsulate NetP subjects within observers (#1436)
  Skip /Applications symlink script for Debug builds (#1443)
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.

2 participants