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.

Sign up for GitHub

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

Jump to bottom

refactor: unnest and simplify oohelperd implementation #2134

Closed
4 tasks
bassosimone opened this issue Jun 8, 2022 · 1 comment
Closed
4 tasks

refactor: unnest and simplify oohelperd implementation #2134

bassosimone opened this issue Jun 8, 2022 · 1 comment
Assignees
@bassosimone
Labels
cleanup There's need to cleanup stuff a bit ooni/probe-engine priority/low refactoring

Comments

@bassosimone
Copy link
Contributor

While working on ooni/probe-cli#806, I noticed that oohelperd's implementation could really use a cleanup and refactoring. Here's what we should roughly do:

  • unnest the implementation (long time ago, I was creating many small interconnected packages while now I have settled on creating a large, flat set of packages)
  • make sure we're using unit tests rather than integration tests
  • we should use mocks from internal/model/mocks rather than rolling out our own mocks
  • if we need integration tests, we should be testing against the oohelper implementation in the same codebase

This refactoring is not very important because the existing code is working ~fine, but would improve the state of the tree.
@bassosimone bassosimone self-assigned this Jun 8, 2022
bassosimone added a commit to ooni/probe-cli that referenced this issue Jul 5, 2022
@bassosimone
refactor(oohelperd): improve tests implementation
34b8389 
After this diff has landed, we have addressed all the points
originally published at ooni/probe#2134.
@bassosimone bassosimone mentioned this issue Jul 5, 2022
Merged
bassosimone added a commit to ooni/probe-cli that referenced this issue Jul 5, 2022
@bassosimone
refactor(oohelperd): improve tests implementation (#835)
d419ed8 
After this diff has landed, we have addressed all the points
originally published at ooni/probe#2134.
@bassosimone
Copy link
Contributor Author

This has been done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bassosimone bassosimone

Labels
cleanup There's need to cleanup stuff a bit ooni/probe-engine priority/low refactoring
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@bassosimone