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

Fix tests #145

Merged
merged 3 commits into from
May 28, 2017
Merged

Fix tests #145

merged 3 commits into from
May 28, 2017

Conversation

mvduin
Copy link
Contributor

@mvduin mvduin commented May 28, 2017

The tests used deepEqual (i.e. deep ==) instead of deepStrictEqual (i.e. deep ===), which inappropriately considers all sorts of things equal that really aren't.

Fixing this uncovered that the test for unmarshalling booleans was broken, which I fixed also.

mvduin added 2 commits May 28, 2017 09:33
plain deepEqual is way too lax!
These tests were plain incorrect, but this went unnoticed due to the
laxness of deepEqual
@mvduin
Copy link
Contributor Author

mvduin commented May 28, 2017

Argh, does anyone really still care about node 0.12 ?

use assert.deepEqual as mediocre workaround for assert.deepStrictEqual
@sidorares
Copy link
Owner

Argh, does anyone really still care about node 0.12 ?

no, I even happy to drop 4 if required. Two last stable version at most and 8.0 is around the corner

@sidorares sidorares merged commit d7002e2 into sidorares:master May 28, 2017
@sidorares
Copy link
Owner

@mvduin thanks for your work! if you feel like you can contribute some more - @nschoe did some amazing work but unfortunately I was extremely busy at a time and it was neglected :( If you can have a look that would be awesome

@mvduin mvduin deleted the fix-tests branch May 28, 2017 09:42
@mvduin
Copy link
Contributor Author

mvduin commented May 29, 2017

Dropping node 0.12 would be a good idea then. A fair amount of ES6 (e.g. 'const' and 'class') should then work provided code uses 'use strict'. This seems to be a cause of test failures for some of the stuff in PR #130 (by @nschoe). His stuff is overall hard to review however because of the messy commits that mix code changes with style or whitespace changes. While I fully agree some cleanup would be welcome (e.g. indentation is really all over the place), this should really be done in a separate commit that performs no functional changes. I've purposely tried to keep my commits clean and offered them in snack-sized pull requests. (Even so, PR #143 is still open...)

Based on a quick look it seems @nschoe's work is mostly related to implementing a dbus service in nodejs... while I might be interested in this in the future, I also have limited time and right now have to prioritize work on dbus client functionality, which is fairly urgent for me. #143 and #144 fix the most glaring demarshalling issues I ran into (although properly deserializing dictionaries as objects (for a{s*}) or Maps (for other a{?*}) would still be nice, and I've seen work along those lines in other PRs). The next areas of attention for me are peer tracking and better support of the common idiom (used e.g. by systemd and avahi) of returning an object and expecting the caller to be immediately able to receive signals on it.

@sidorares
Copy link
Owner

Thanks @mvduin

Client side is more important for me as well, I'd like to use more this library as desktop automation tool (avahi, network manager, unity/gnome/kde integrations)
Though at protocol level it's quite symmetrical due to bus architecture, top level APIs might end up quite different.

I also thinking about splitting this into core protocol ( basically just sending / receiving messages ) and everything built on top ( exposing / consuming services and standard dbus interfaces )

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.

2 participants