Skip to content

Conversation

@Velin92
Copy link
Member

@Velin92 Velin92 commented Nov 18, 2025

This PR allows to test the notification content after it has been processed by the NSE, using UnitTests.

It contains 2 major project changes

NSE Files references

All NSE files are also referenced by Element X (the main app).
The reason for this is to allow generating mocks, but most importantly because due to a limitation of the Unit Tests, importing as testable the NSE is not possible, the compiler will not flag any error because it's able to find the definitions of the files... but will fail when building because it's not able to compile and link their content, since for some reason the UnitTests do not include the sources of app extensions directly, but just their signatures.
The only solution is then to have also the NSE code referenced in Element X so that the testable import resolves and compiles its sources.

Dynamic Swift Package

Use of the swift package Dynamic.
It allows to access private APIs from Apple... I know that sounds crazy, but the package is only used in UnitTests and it's not supposed to be used in the production code of the main app.
But what is the reason to do so, you might ask. The reason is simple, we use the quite obscure Communication Notifications APIs for instant messaging, these do a lot of stuff behind the curtains.
For example the title and the subtitle of the notification content are completely ignored when rendering the notification, and the values provided when generating the INIntent to render the icon, set the sender, and the group of the message belongs to (if available) get used in its place.
This all gets stored in a private member of the notification content called communicationContext, and this member is not normally accessible since it's supposed to be used by the OS to render the notification as a communication notification. However there is a ton of logic behind customising the icon, the names displayed for the sender and/or the room based on mentions, threads, DMs and so on.
Accessing the communicationContext values is the best way to verify that the notification is formatted in the proper way and manner.

Other changes

  • Moved some files, like the NotificationItemProxy into the NSE folder
  • Renamed addSeconderIcon to addCommunicationContext since the old name was misleading, and made it seem like the function just added the icon, when in reality it also controls the rendering of the title and subtitle.

What is missing?

Would be even nicer if we could get the icon file/image path... but I can't seem to find where it's referenced in the notification content. We could in this way check that the correct placeholder avatar is displayed, or if the correct avatar is used when available. But I am starting to think that is not at all present in the notification content itself, and that the OS does some stuff behind the curtains to handle that.

@Velin92 Velin92 force-pushed the mauroromito/test_nse branch from b492607 to f07410e Compare November 18, 2025 15:12
@Velin92 Velin92 added the pr-misc for other changes label Nov 18, 2025
@codecov
Copy link

codecov bot commented Nov 18, 2025

❌ 2 Tests Failed:

Tests completed Failed Passed Skipped
1107 2 1105 0
View the top 1 failed test(s) by shortest run time
PreviewTests::testRoomMemberDetailsScreen()
Stack Traces | 2.79s run time
failed - Snapshot "Other User-iPhone-pseudo" does not match reference.

@−
"file:.../__Snapshots__/PreviewTests/roomMemberDetailsScreen.Other-User-iPhone-pseudo.png"
@+
"file:.../tmp/PreviewTests/roomMemberDetailsScreen.Other-User-iPhone-pseudo.png"

To configure output for a custom diff tool, use 'withSnapshotTesting'. For example:

    withSnapshotTesting(diffTool: .ksdiff) {
      // ...
    }

The percentage of pixels that match 0.94210815 is less than required 1.0
The lowest perceptual color precision 0.29250002 is less than required 0.98 (PreviewTests/Sources/PreviewTests.swift:115)
View the full list of 2 ❄️ flaky test(s)
DeferredFulfillmentTests::testObservableAsynchronousUpdate()

Flake rate in main: 32.71% (Passed 253 times, Failed 123 times)

Stack Traces | 10.1s run time
Asynchronous wait failed: Exceeded timeout of 10 seconds, with unfulfilled expectations: "Awaiting stream". (.../Other/Extensions/XCTestCase.swift:77)
UserSessionFlowCoordinatorTests::testRoomPresentation()

Flake rate in main: 40.13% (Passed 94 times, Failed 63 times)

Stack Traces | 1.88s run time
XCTAssertTrue failed (UnitTests/Sources/UserSessionFlowCoordinatorTests.swift:80)

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@Velin92 Velin92 force-pushed the mauroromito/test_nse branch from c4c0364 to c8a89d5 Compare November 18, 2025 18:40
@Velin92 Velin92 marked this pull request as ready for review November 18, 2025 18:40
@Velin92 Velin92 requested a review from a team as a code owner November 18, 2025 18:40
@Velin92 Velin92 requested review from pixlwave and stefanceriu and removed request for a team November 18, 2025 18:40
@Velin92
Copy link
Member Author

Velin92 commented Nov 18, 2025

The tests take 0.5 locally... but sometimes on CI they may take more than 1 minute causing the test to fail.
It's probably due to the rendering of the placeholder image just taking too long on CI. Since we are not able to get the image anyway for testing, maybe it's worth just skipping that part in the code when we are running a test?

@Velin92 Velin92 force-pushed the mauroromito/test_nse branch from 0bf249d to 861fecd Compare November 19, 2025 10:51
Copy link
Member

@stefanceriu stefanceriu left a comment

Choose a reason for hiding this comment

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

Talked about it elsewhere, great stuff! 👏

@Velin92 Velin92 force-pushed the mauroromito/test_nse branch from 861fecd to 777cad2 Compare November 19, 2025 11:24
@Velin92 Velin92 enabled auto-merge (rebase) November 19, 2025 11:25
@sonarqubecloud
Copy link

@Velin92 Velin92 disabled auto-merge November 19, 2025 12:20
@Velin92 Velin92 merged commit 21249f7 into develop Nov 19, 2025
7 of 8 checks passed
@Velin92 Velin92 deleted the mauroromito/test_nse branch November 19, 2025 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-misc for other changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants