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

Unit tests draft #100

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Unit tests draft #100

wants to merge 6 commits into from

Conversation

aileo
Copy link
Contributor

@aileo aileo commented Jul 3, 2020

Attempt to create a test suite to check logic without physical devices.

  • mock BLE device to send/receive messages
  • for each lpf2 hub:
    • check connection process
    • check supported version control
  • for each lpf2 hub device:
    • check device attachment

Also fix _requestHubPropertyValue so it will reject if _parseHubPropertyResponse throws.

@nathankellenicki nathankellenicki marked this pull request as ready for review July 30, 2020 20:44
@nathankellenicki
Copy link
Owner

@aileo Hi, apologies for taking a while to get to this, I've taken some time off this project to work on a couple of other things - and work is busy at the moment. :)

I tried this PR, and it's awesome! I wanted to add unit tests but never gotten round to it, so it's awesome you did this!

To be honest, I'm perfectly happy merging this as is - it works great, tests pass, and it adds a lot to the project.

The only thing is that when I run npm run build:all it complains about test/test.ts existing outside of rootDir. :)

@aileo
Copy link
Contributor Author

aileo commented Jul 31, 2020

Hi, apologies for taking a while to get to this, I've taken some time off this project to work on a couple of other things - and work is busy at the moment. :)

Very nice tchoo-tchoo videos :)

I tried this PR, and it's awesome! I wanted to add unit tests but never gotten round to it, so it's awesome you did this!

To be honest, I'm perfectly happy merging this as is - it works great, tests pass, and it adds a lot to the project.

The only thing is that when I run npm run build:all it complains about test/test.ts existing outside of rootDir. :)

I'm happy if you're happy :D
I tried to deal with the compiling error, but it ended not compiling at all so I am asking for support :)
I think I should write tests for at least one device before merging so it shows what is expected if anyone wants to contribute... But I don't know myself where to start, maybe the simplest ones (current, voltage, hub button and hub led), what do you think ?

@nathankellenicki
Copy link
Owner

Very nice tchoo-tchoo videos :)

I decided to have some fun and actually write some code that uses this library for once. ;)

I think I should write tests for at least one device before merging so it shows what is expected if anyone wants to contribute... But I don't know myself where to start, maybe the simplest ones (current, voltage, hub button and hub led), what do you think ?

I think that's a great idea. Perhaps one output device and one input device as a demonstration of both? I think Hub Button and Hub LED would both be good candidates for this.

@aileo aileo mentioned this pull request Aug 3, 2020
- improve sent message check
- remove temperature subscription test on technicmediumhub
- replace equal by strictEqual due to deprecation
@aileo
Copy link
Contributor Author

aileo commented Jan 25, 2021

Hi, sorry for the delay, I finally got back on this lib.

I think that's a great idea. Perhaps one output device and one input device as a demonstration of both? I think Hub Button and Hub LED would both be good candidates for this.

I wrote tests for the HubLED Device but the Hub buttons are not devices so I will go for the remotecontrolbutton instead.

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