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

epic: Testing #85

Closed
2 of 3 tasks
mircea-pavel-anton opened this issue Oct 13, 2024 · 3 comments
Closed
2 of 3 tasks

epic: Testing #85

mircea-pavel-anton opened this issue Oct 13, 2024 · 3 comments
Assignees

Comments

@mircea-pavel-anton
Copy link
Collaborator

mircea-pavel-anton commented Oct 13, 2024

After merging some renovate updates that passed the (extremely minimal) testing that was in place, the app has some issues when importing DNS entries from MTK API. As such, better testing is definitely required.

Goals and non-goals

  • I only plan to test the code that is directly interacting with the Mikrotik API
  • For unit tests, I will mock the mikrotik api with some dummy data
  • I do NOT plan to test anything under pkg/webhook as it is simply copy pasted from other providers and I have absolutely 0 clue as to what that code does 😅
  • There are some tests in place for internal/configuration and internal/logging. I do NOT plan to expand those
  • I have no clue as to what I should test under internal/dnsprovider

TODO

@mircea-pavel-anton
Copy link
Collaborator Author

Relevant: #88

@mircea-pavel-anton
Copy link
Collaborator Author

mircea-pavel-anton commented Oct 16, 2024

Questions

Client Connection Validation

Should I validate the connection between the API Client and the actual RouterOS API server as part of the Client creation, Provider creation or neither?

I think it is important to validate the client can connect properly (good credentials, good url etc.) sooner rather than later so that we can fail fast if something is wrong.

48c00db

@mircea-pavel-anton
Copy link
Collaborator Author

after implementing #91 and #94 as well as adding relevant tests for #98 #97 and #95 , I am much more confident in this code. I will close this for now, as I feel like at least the intention of the original issue has been addressed. Obviously, there is still room for improvement but that will (probably?) come later on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant