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

Track file not updated #7

Merged
merged 9 commits into from May 23, 2015

Conversation

Projects
None yet
2 participants
@alfateam123
Copy link
Contributor

alfateam123 commented May 23, 2015

Fixes alfateam123/Miyuki#2
The :git parameter in the gemfile for hydride0/yamazaki is needed to test Miyuki on Travis with the needed patches to make it work. It will be removed when @hydride0 will merge hydride0/yamazaki#11

alfateam123 added some commits May 16, 2015

Catch exceptions when notifying
I'm testing on a debian server. I didn't install libnotify, as
it requires gnome (`dnw`).

Tests have to notify things, and the lack of libnotify
raises exceptions that break automated tests on my platform
(when executed with `rake test`).
@RoxasShadow

This comment has been minimized.

Copy link
Owner

RoxasShadow commented May 23, 2015

Thanks mate. I'll wait for hydride0/yamazaki#11 be merged so we can rid off those references.

@@ -13,5 +13,5 @@
##

module Miyuki
VERSION = '0.5.8.1'
VERSION = '0.5.8.2'

This comment has been minimized.

@RoxasShadow

RoxasShadow May 23, 2015

Owner

This is ok, but usually is good practice leaving to the manteiner the bump of the version 💃

This comment has been minimized.

@alfateam123

alfateam123 May 23, 2015

Author Contributor

Oh :///:

@notifier.notify(title, message) if has_notifier?
rescue NoMethodError
#libnotify is not installed, maybe.
#someone may not want to install gnome on its server :V

This comment has been minimized.

@RoxasShadow

RoxasShadow May 23, 2015

Owner

That someone can just turn off notifications tho.

This comment has been minimized.

@alfateam123

alfateam123 May 23, 2015

Author Contributor

May I remove notifications from test' configurations? If we need to test them, we need them turned on, and rake test will die if libnotify (on servers, for example) is not installed.
I'd prefer not to modify them a lot, as they may invalidate the test phase :V

This comment has been minimized.

@RoxasShadow

RoxasShadow May 23, 2015

Owner

Sure you can.

This comment has been minimized.

@alfateam123

alfateam123 May 23, 2015

Author Contributor

I'd prefer not to touch them, I'll just do a .patch file to disable notifications when needed (manual operation, you know what you're doing). It's another file to maintain, but I don't think it's a problem, as they can be generated (and applied) with just a command.
Should I remove this begin ... rescue block too?

Still, the problem remains: if we implement the test steps related to notifications (this and that ), they have to be executed somehow to make the tests pass.

This comment has been minimized.

@RoxasShadow

RoxasShadow May 23, 2015

Owner

Tests related to notifications are not implemented yet since I need to find a way to perform them. I suggest you to turn off the notifications for now (yes, removing also begin ... rescue).

alfateam123 added some commits May 23, 2015

Remove begin ... rescue block from notifications
the `Notifier#notify` call was wrapped in a `begin ... rescue`
block in order to fix tests.

In case you launch tests on a platform where `libnotify` is not
installed, you can apply the patch added with this commit.
Refer to the new `TROUBLESHOOTING.md` file to find fixes
in case you get errors.
@RoxasShadow

This comment has been minimized.

Copy link
Owner

RoxasShadow commented May 23, 2015

👍

RoxasShadow added a commit that referenced this pull request May 23, 2015

@RoxasShadow RoxasShadow merged commit 6b2e80e into RoxasShadow:master May 23, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.