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

Suggested fixes for issues #18, #19, and #24 #25

Merged
merged 5 commits into from Oct 3, 2015

Conversation

mwittig
Copy link
Collaborator

@mwittig mwittig commented Aug 11, 2015

See commit log for details

@philip1986
Copy link
Owner

I think we reach a degree of complexity were we need tests to ensure the stability of our code. I already thought about an approach for tests like this, which takes also in account that you like to split the plugin by devices . So I will try to add some basic tests.

@philip1986
Copy link
Owner

https://github.com/philip1986/pimatic-led-light/pull/25/files#diff-fc25f60a59d04b30ef5515738aff8bc6R55
this line is also broken @power is either 'on' or 'off', so the condition is always true.

@mwittig
Copy link
Collaborator Author

mwittig commented Aug 11, 2015

Agree on testing :)
Note, however, the power attribute should be true/false to get properly written to the database as far as I know. The dummy and blinkstick device implementations also use true/false btw.

@philip1986
Copy link
Owner

Yeh we need to refactor this on/off implementation, its also quite confusing I just realized it during writing the tests.
So this PR is OK.

@philip1986
Copy link
Owner

@mwittig checkout #26.
This is what I have so far in terms of testing. please let me know what you thing about this approach.

Some tests are skipped because of broken code. This the includes also a test for setColor on milight, which should be fixed by this PR.

@mwittig
Copy link
Collaborator Author

mwittig commented Aug 20, 2015

Hi @philip1986 I had a frist look at your test effort which I really like. what do I need to checkout to run the tests? Can I simply chckou the "feature/add_tests" branch?

@philip1986
Copy link
Owner

@mwittig ok than I will go on whit this tests. Yes just checkout the branch and run npm install.

@mwittig
Copy link
Collaborator Author

mwittig commented Sep 7, 2015

@philip1986 Added suggested fix for issue #19

@mwittig mwittig changed the title Suggested fixes for issues #18 and #24 Suggested fixes for issues #18, #19, and #24 Sep 7, 2015
philip1986 pushed a commit that referenced this pull request Oct 3, 2015
Suggested fixes for issues #18, #19, and #24
@philip1986 philip1986 merged commit 9dc9e4c into philip1986:master Oct 3, 2015
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.

None yet

2 participants