-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[flutter_local_notifications_windows] Fix URIs on Windows -- images and audio #2511
Conversation
Not sure why the CI is failing -- here's my local output: What would you like me to do? flutter test -r expanded
00:00 +0: loading D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/bindings_test.dart
00:00 +0: D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/bindings_test.dart: Bindings (setUpAll)
00:00 +0: D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/bindings_test.dart: Bindings work in simple cases
00:00 +1: D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/bindings_test.dart: Bindings fail when ID is not found in simple cases
00:00 +2: D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/bindings_test.dart: Bindings are included in show()
00:00 +3: D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/details_test.dart: Details: (setUpAll)
00:00 +3: D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/bindings_test.dart: Bindings fail when notification has been cancelled
00:00 +4: D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/bindings_test.dart: Bindings fail when notification has been cancelled
00:00 +5: D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/details_test.dart: Details: Simple details
00:00 +6: D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/plugin_test.dart: Plugin (setUpAll)
00:00 +6: D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/details_test.dart: Details: Actions
00:00 +7: D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/plugin_test.dart: Plugin initializes safely
00:00 +8: D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/details_test.dart: Details: Audio
00:00 +9: D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/details_test.dart: Details: Audio
00:00 +10: D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/plugin_test.dart: Plugin cannot be used before initializing
00:00 +11: D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/details_test.dart: Details: Rows
00:00 +12: D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/details_test.dart: Details: Rows
00:00 +13: D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/details_test.dart: Details: Rows
00:00 +14: D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/scheduled_test.dart: Schedules (setUpAll)
00:00 +15: D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/scheduled_test.dart: Schedules (setUpAll)
00:00 +16: D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/scheduled_test.dart: Schedules (setUpAll)
00:00 +16: D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/details_test.dart: Details: Inputs
00:00 +17: D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/scheduled_test.dart: Schedules work with basic times
00:00 +18: D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/details_test.dart: Details: Progress
00:00 +19: D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/details_test.dart: Details: Progress
00:00 +20: D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/details_test.dart: Details: Progress
00:00 +20 -1: D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/details_test.dart: Details: Progress [E]
Expected: NotificationUpdateResult:<NotificationUpdateResult.success>
Actual: NotificationUpdateResult:<NotificationUpdateResult.notFound>
package:matcher expect
test\details_test.dart 243:11 main.<fn>.<fn>
Retry: Details: Progress
00:01 +21: D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/details_test.dart: Details: (tearDownAll)
00:01 +21: All tests passed! |
flutter_local_notifications_windows/lib/src/details/notification_audio.dart
Outdated
Show resolved
Hide resolved
I'm not sure why it's failing as well but seems like some tests are failing to completely run |
Actually from the output you've shared from running it locally, it still has an exit code of 1 so that would be why. Not sure why that is happening but if that's fixed locally then that would most likely fix the issue |
I went back and re-worked things slightly based on your action-image comment: both I also updated my previous message with my actual (successful) test output on my own laptop. The GitHub version is weird -- despite saying I'm also open to renaming everything |
I'm going to push a change to temporarily enable verbose logging on the tests to see if it might provide more info on what's going on |
Actually I probably won't need to do that anymore. I've pasted the output below
The summary says all tests pass but it actually picked up differences between the expected and actual values. I could be wrong as it's been a while but I don't recall this being an issue before. Perhaps this is the reason why GitHub found issues? Edit: actually the output from GitHub is green for this so perhaps isn't related. I'll push verbose logging anyway |
Well, I didn't get anything interesting by checking out the lines the stack trace is referring to. The tests pass, no errors occur, but the exit code is being set somewhere to a non-zero value. But it's using the global variable so it's a little hard to follow the logic. |
I also took a look at the verbose logs and looks like the tooling is crapping out. Not sure why but may have to ignore this for now. As for verifying the changes in the PR, I've tried to create the MSIX for the example but can't install it. I changed the MSIX config to install the cert too but that didn't help. Any pointers? |
If you're getting the "untrusted certificate" error, or something like that when trying to install a valid msix, that's normal:
It seems to be another deterrent to stop people from running unsigned code. Users who actually have a valid and not just self-signed MSIX certificate won't run into these issues (I might consider opening a PR to |
I figured out the flaky test problem! Locally, the tests run together. On CI, the tests are interrupted and the process is shut down early. Checking the Dart and C++ code, and cross-referencing that with what else works on CI, my guess is that one of these two lines is causing the crash: ScheduledToastNotification notification(doc, winrt::clock::from_time_t(time));
plugin->notifier.value().AddToSchedule(notification); And somehow the Flutter tooling isn't picking it up as a failed test. Then, when it reaches the end and notices that a test didn't finish properly somewhere, it exits with a failure. I'll try to report this to the Flutter team soon, but for now I deleted that test, so everything passes now. I was also able to get Broken test// test/scheduled_test.dart
test('work with basic times', skip: true, () async {
await plugin.cancelAll();
expect(await countPending(), 0);
final TZDateTime now = TZDateTime.now(location);
final TZDateTime later = now.add(const Duration(days: 1));
expect(plugin.zonedSchedule(300, null, null, later, null), completes);
expect(await countPending(), 1);
expect(plugin.zonedSchedule(301, null, null, later, null), completes);
expect(await countPending(), 2);
expect(plugin.zonedSchedule(302, null, null, later, null), completes);
expect(await countPending(), 3);
await plugin.cancelAll();
expect(await countPending(), 0);
}); Local output[ +6 ms] test 3: Started flutter_tester process at pid 20056
[ +180 ms] test 3: connected to test device, now awaiting test result
[ +1 ms] test 3: Waiting for test harness or tests to finish
✅ D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/scheduled_test.dart: Schedules work with basic times
✅ D:/Flutter/Packages/flutter_local_notifications/flutter_local_notifications_windows/test/scheduled_test.dart: Schedules do not work with earlier time
[ +191 ms] test 3: Test harness is no longer needed by test process
[ ] test 3: finished
[ +1 ms] test 3: cleaning up...
[ ] test 3: ensuring test device is terminated.
[ ] test 3: Terminating flutter_tester process
[ ] test 3: Shutting down DevTools server
[ ] test 3: Test process is no longer needed by test harness
[ ] test 3: Shutting down test harness socket server
[ +6 ms] test 3: flutter_tester process at pid 20056 exited with code=-1
[ ] test 3: deleting temporary directory
[ +2 ms] test 3: finished CI output[ +2 ms] test 3: Started flutter_tester process at pid 4008
[ +189 ms] test 3: connected to test device, now awaiting test result
[ ] test 3: Waiting for test harness or tests to finish
✅ d:/a/flutter_local_notifications/flutter_local_notifications/flutter_local_notifications_windows/test/scheduled_test.dart: Schedules work with basic times
[+1226 ms] test 3: Test process is no longer needed by test harness
[ ] test 3: finished
[ ] test 3: cleaning up...
[ ] test 3: ensuring test device is terminated.
[ ] test 3: Terminating flutter_tester process
[ ] test 3: Shutting down DevTools server
[ ] test 3: Test harness is no longer needed by test process
✅ d:/a/flutter_local_notifications/flutter_local_notifications/flutter_local_notifications_windows/test/scheduled_test.dart: Schedules do not work with earlier time
[ ] test 3: Shutting down test harness socket server
[ ] Compiling file:///C:/Users/RUNNER~1/AppData/Local/Temp/flutter_tools.bce9bb2d/flutter_test_listener.20e0f0d4/listener.dart
[ +3 ms] test 3: flutter_tester process at pid 4008 exited with code=-1073740791
[ ] test 3: deleting temporary directory
[ +1 ms] test 3: finished I also updated the example, so feel free to merge. The Android integration test should just need a re-run |
flutter_local_notifications_windows/lib/src/details/notification_parts.dart
Outdated
Show resolved
Hide resolved
Whilst PR is green I'm getting the following output from running the tests locally
|
Interesting, mine works fine: $ flutter test -j 1
00:03 +20: All tests passed! Google searches indicate you're trying to load a 32-bit DLL -- can you delete it and try running |
Interesting. Not sure how you were doing that before but I can send you the 64-bit DLL if you need.
Thanks 🤦, removed from the C++ side as well |
expect(plugin.showRawXml(id: 5, xml: complexXml), completes); | ||
}); | ||
test('catches invalid XML', () async { | ||
expect(plugin.isValidXml(emptyXml), isFalse); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something I picked up after merging but these are all meant to be separate tests as there's a different permutations of input and output but have made as part of a single test()
. The description given is also misleading as there are tests that assert valid XML was provided and is more appropriate as a group description. I noticed there are other test classes that are like this too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I plan to re-organize the tests, and have more thorough ones. For example, while there are tests to make sure the APIs can distinguish between valid and invalid XML, and tests to make sure the WindowsNotificationsDetails
generates valid XML, there are no tests to map WindowsNotificationDetails
to specific XML, eg, to verify that a progress bar is actually inserted. Another PR for another day :)
Windows custom audio and image support wasn't very well tested, and #2510 reported that it doesn't even work in its current state.
This PR:
WindowsNotificationAudio.asset
, for MSIX apps onlyWindowsImage()
, for all sorts of URIs, with a big page of documentationFixes #2510