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

Changes #17

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Changes #17

wants to merge 2 commits into from

Conversation

Master-Silver
Copy link

Hi, 
just take a look on these changes. I use your API for a guest WLAN portal, but for my needs I had to do some adjustments on it. Maybe some of it will interest you. I'm still not finished with my work but one thing I noticed was, if the connection to the network controller is not used for some time, the controller will reject new requests with a 401 and there is no way of knowing before that happens, that you have to authenticate again.

Use different unifisites on one connection simultaneously.
@KoenZomers
Copy link
Owner

Hi @Master-Silver, thanks for sharing your work! If I understand it correctly, it's adding a fakes framework to allow unit testing without having a UniFi controller present to test against, correct? While I certainly do see how this could be useful as the current UnitTests would be changing device settings and changing them back again on a live UniFi controller, it doesn't seem to be complete and adds some level of complexity into the mix. I'm not sure if that's worth it for such a simple project to be honest.

I'll have a look at the 401s if I can find a decent way to deal with that.

Thanks for sharing again and please correct me if I have misunderstand your contribution.

@KoenZomers KoenZomers self-assigned this Feb 1, 2021
@KoenZomers KoenZomers added the Reviewing Item is being reviewed label Feb 1, 2021
@KoenZomers
Copy link
Owner

Just added code that should deal with the 401 scenario you described. It's in the latest release.

@Master-Silver
Copy link
Author

Hi @KoenZomers,

yes the idea was to have unit tests that not need a real controller to work. I use your API in my project and the other components in my project have all unit tests (282 together at the moment). At some point of time it could be possible that I return to your API and write unit tests for the parts I use. I just wanted to know if you are interested in it. But either way I would definitely advise to get the Interface for the API, I needed it to write a fake API for my other components unit tests.
For the scale of my project, I work for CineStar at the moment, and they use much UniFi hardware. So for me there is no way around unit tests.

Have a nice day
Master Silver

@KoenZomers
Copy link
Owner

Thanks for elaborating @Master-Silver. It makes sense what you're saying. I fixed the automated build for this project in GitHub and then also came to realize that the unit tests in the build could never work without having something like fakes. If time permits you, could you rebase this branch on the current master and implement it a bit further so I can see and learn from you how you would implement this?

@Master-Silver
Copy link
Author

@KoenZomers, sure I really like to do that. At the moment I have to get the project I am working on to a stable release state, but after that there is a good change I have time to working on your API again.
Keep your self save until that, it is really much happening in the world right now.

@KoenZomers
Copy link
Owner

@Master-Silver Curious to hear if you have found a chance to continue with your proposal as discussed here.

@Master-Silver
Copy link
Author

@KoenZomers sadly not, but I still intend to do that. Maybe there will soon be time for it, but at the moment I'm still busy with other parts of the project which I mentioned in my previous posts.

@Master-Silver
Copy link
Author

Master-Silver commented Jul 7, 2021

Hi @KoenZomers,

I found some time and added your changes on the master to my fork. I did make a new branch for the unit tests with fake classes. It was cleaner than to rebase on the old branch. Take a look https://github.com/Master-Silver/UniFiApi/tree/unit-tests-with-fakes

@KoenZomers
Copy link
Owner

Hi @KoenZomers,

I found some time and added your changes on the master to my fork. I did make a new branch for the unit tests with fake classes. It was cleaner than to rebase on the old branch. Take a look https://github.com/Master-Silver/UniFiApi/tree/unit-tests-with-fakes

Awesome! I'll have a look at it. Appreciate taking the time to create it.

@Master-Silver
Copy link
Author

@KoenZomers had you any time to take a look?

@KoenZomers
Copy link
Owner

Not yet unfortunately, it's on my todo list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting feedback Reviewing Item is being reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants