Skip to content

Conversation

przemyslaw-zan
Copy link
Member

@przemyslaw-zan przemyslaw-zan commented May 16, 2023

Feature (tests): Added karma notification for when the tests are completed. Notification can be enabled with -n/--notify CLI option. Its enabled by default when running all tests, without the -f/--files option. Closes ckeditor/ckeditor5#13736.


Additional information

(This happened on Windows 10)

I've encountered a small issue where the first attempt to use the notifier did not work, and only following attempts worked successfully. It happened when I've started with hard-coded value: reporters: [ 'progress', 'notify' ], so it might not be an issue.

Reviewers should make sure that the current setup works on first try.

@coveralls
Copy link

coveralls commented May 16, 2023

Coverage Status

Coverage: 87.903% (-0.3%) from 88.214% when pulling 11edec3 on ck/13736 into a641d0f on master.

Copy link
Contributor

@martnpaneq martnpaneq left a comment

Choose a reason for hiding this comment

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

Works fine on mac :)
image

The only issue I had was that I rejected the notification on the first run, and I could no longer see any notifiactions.
In this case resetting notification settings for terminal fixed it for me:
image

@arkflpc
Copy link
Contributor

arkflpc commented May 17, 2023

Works on Linux under KDE.

Copy link
Contributor

@psmyrek psmyrek left a comment

Choose a reason for hiding this comment

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

Notification works in Windows 11:

Zrzut ekranu 2023-05-17 134720

The not cool thing is that it adds a shortcut in the Start menu...

Zrzut ekranu 2023-05-17 134842

obraz

Could you please check if this could be prevented somehow?

@psmyrek
Copy link
Contributor

psmyrek commented May 18, 2023

Possible workaround:

const notifier = require( 'node-notifier' );
const isWindowsNotifier = notifier.Notification === notifier.WindowsToaster;

if ( isWindowsNotifier ) {
	const originalNotify = notifier.notify;

	notifier._notify = notifierOptions => originalNotify( {
		...notifierOptions,
		appID: 'karma-notify-reporter'
	} );
}

@przemyslaw-zan
Copy link
Member Author

☝️ To avoid workarounds, we've decided to implement a custom notifier instead.

@przemyslaw-zan
Copy link
Member Author

Right now notifications use default icon. Do we want to use custom icons, and if so, where do we take them from? Should we have different icons for success and failure, or perhaps simple CKE5 logo for all notifications would be sufficient?

image

Copy link
Member

@pomek pomek left a comment

Choose a reason for hiding this comment

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

LGTM.

@pomek pomek merged commit d1a5898 into master May 19, 2023
@pomek pomek deleted the ck/13736 branch May 19, 2023 08:49
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.

Could Karma send a notification once finished processing tests?
6 participants