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

[WIP] Add tests #8

Closed
wants to merge 44 commits into from
Closed

[WIP] Add tests #8

wants to merge 44 commits into from

Conversation

YurySolovyov
Copy link

@YurySolovyov YurySolovyov commented Aug 14, 2016

I thought it would be nice to start a PR right away, so we can discuss everything as we go.

Some notes:

  • fixed old image thumb, we should really find more reliable image with persistent url
  • split up interface creators
  • I started with jasmine as I thought it has pretty lightweight setup and everything I need ATM.

Feel free to leave a comment or commit some stuff as I will squash everything at the end.

Fixes #7

package.json Outdated
@@ -4,7 +4,7 @@
"description": "Node.js implementation for the MPRIS D-Bus Interface Specification to create a mediaplayer service",
"main": "index.js",
"scripts": {
"test": "echo \"Error: no test specified\" && exit 1"
"test": "./node_modules/.bin/jasmine"
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can just write jasmine, NPM will expand it to node_modules/.bin/jasmine automatically.

@emersion
Copy link
Collaborator

Looks very good! We can also get rid of all the ugly _create*Interface methods by using Symbol() and use ES6 since this thing will only be used in Node.

Also, in the tests what happens if done is never called? Is there a timeout?

@YurySolovyov
Copy link
Author

Some questions:

  • do you have any particular minimal node version for this? I mean if you are ok with targeting node 4 LTS, we could use some ES6 here like const/let, classes, Promises and so on.
  • I wonder if we can set up a CI to run the tests, though I'm not sure if we will be able to provide all dbus deps for it.

@YurySolovyov
Copy link
Author

YurySolovyov commented Aug 14, 2016

Is there a timeout?

Yes

@emersion
Copy link
Collaborator

do you have any particular minimal node version for this? I mean if you are ok with targeting node 4 LTS, we could use some ES6 here like const/let, classes, Promises and so on.

I think just targeting LTS will be fine. This module was written a long time ago, and needs ES6.

I wonder if we can set up a CI to run the tests, though I'm not sure if we will be able to provide all dbus deps for it.

I think this can work. Will try to do it.

.travis.yml Outdated
apt:
packages:
- gcc-4.8
- g++-4.8
Copy link
Author

Choose a reason for hiding this comment

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

I guess this needs everything from https://github.com/Shouqun/node-dbus#dependencies plus libexpat1-dev

@YurySolovyov
Copy link
Author

Comment got hidden:

I guess this needs everything from https://github.com/Shouqun/node-dbus#dependencies plus libexpat1-dev

@emersion
Copy link
Collaborator

emersion commented Aug 14, 2016

Note: if we want to rollback to precise on travis, we need:

env:
  - CC=gcc-5 CXX=g++-5
addons:
  apt:
    sources:
      - ubuntu-toolchain-r-test
    packages:
      - gcc-5
      - g++-5

@emersion
Copy link
Collaborator

Yay! The build now works :D

},
waitForEvent: (player, event) => {
return new Promise((resolve) => {
player.on(event, resolve);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should use .once() here to do not end up with a large number of listeners.

@YurySolovyov
Copy link
Author

YurySolovyov commented Aug 14, 2016

We can also get rid of all the ugly _create*Interface methods by using Symbol()

TBH, I don't like using Symbol. How about just making them classes and using them like so:

this.interfaces = {
  root: new RootInterface()
};
if (this.supportedInterfaces.includes('player')) {
    this.interfaces.player = new PlayerInterface();
}
if (this.supportedInterfaces.include('trackList')) {
    this.interfaces.trackList = new TrackListInterface();
}
if (this.supportedInterfaces.include('playlists')) {
    this.interfaces.playlists = new PlaylistsInterface();
}


return promise.then(() => {

return helpers.getInterfaceAsync(service, objectpath, 'org.mpris.MediaPlayer2.Player').then(obj => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to get the interface outside the reduce?

Copy link
Author

Choose a reason for hiding this comment

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

good catch, will take a look

@YurySolovyov
Copy link
Author

@emersion any news here?

@emersion
Copy link
Collaborator

Rebased all commits (but I should have just merged, too late :( ) with the latest fix in master. I get this:

Started
...*process 13125: arguments to dbus_message_iter_append_basic() were incorrect, assertion "_dbus_check_is_valid_path (*string_p)" failed in file dbus-message.c line 2758.
This is normally a bug in some application using the D-Bus library.
  D-Bus not built with -rdynamic so unable to print a backtrace
fish: “npm run test” terminated by signal SIGABRT (Abort)

@YurySolovyov
Copy link
Author

You might want to show up in gitter :)

@emersion
Copy link
Collaborator

emersion commented Mar 4, 2019

Superseded by #15

@emersion emersion closed this Mar 4, 2019
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.

Add tests
2 participants