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

Run driver tests #97

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Run driver tests #97

wants to merge 1 commit into from

Conversation

mhemeryck
Copy link
Contributor

Was just checking changes to my fork and the upstream one and noticed that the CI setup changed.

Would it be OK to also run the driver tests I did create at the time?

@sde1000
Copy link
Owner

sde1000 commented Jan 26, 2022

I had a few reasons for leaving out driver tests by default. (And also leaving out the flake8 step.)

  1. The drivers aren't part of the supported API of the library (at the moment; I do have a plan, I'm just being very slow)
  2. Not all drivers have tests; only one of them does so far!
  3. Driver tests have additional dependencies, whereas the main set of unit tests only depend on the Python standard library

These are not necessarily all good reasons not to run them. Running them as an additional step, as your commit does, should be fine but may need looking at again in the future.

@mhemeryck
Copy link
Contributor Author

Hey, thanks for your reply!

I wouldn't necessarily mind if would leave these tests out for now. I was intending on looking into this library again since I wanted to augment my DALI-lighting setup. Since I noticed there were some changes since the last time I had looked at your library and since I did notice that the tests didn't run as part of the default test suite, I figured to at least run those again -- and I was glad to notice they still ran fine 🙂

Feel free to close this PR for now. If I find the time, I might work a bit further on the unipi driver in another PR.

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