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

Add initial tests and typings for the Kiosk browser proxy #143

Merged
merged 5 commits into from
Nov 16, 2023
Merged

Add initial tests and typings for the Kiosk browser proxy #143

merged 5 commits into from
Nov 16, 2023

Conversation

guyonvarch
Copy link
Member

@guyonvarch guyonvarch commented Nov 7, 2023 •
edited
Loading

Noticing in #142 that the code reading services was not so clear. This clarify this and also add unit tests and typing tests.

@guyonvarch guyonvarch force-pushed the document-connman-service branch 2 times, most recently from 8742fac to d198ec6 Compare November 8, 2023 11:10
@guyonvarch
Add tests for updated functions in proxy
6058d0b 
- test_has_service_state_in
- test_extract_manual_proxy
@guyonvarch guyonvarch changed the title Document connman service Document connman service and add initial tests for the Kiosk browser Nov 8, 2023
@guyonvarch guyonvarch marked this pull request as ready for review November 8, 2023 13:46
@guyonvarch guyonvarch added the reviewable Ready for initial or iterative review label Nov 8, 2023
knuton
knuton reviewed Nov 9, 2023
View reviewed changes
Copy link
Member

@knuton knuton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Some questions geared towards long term ease of maintenance.

testing/run Outdated Show resolved Hide resolved
kiosk/kiosk_browser/proxy.py Outdated Show resolved Hide resolved
kiosk/kiosk_browser/test_proxy.py Outdated Show resolved Hide resolved
@knuton knuton added details needed Further information requested to better evaluate changes and removed reviewable Ready for initial or iterative review labels Nov 9, 2023
@guyonvarch
Copy link
Member Author

Using a struct which is enouch for our needs, I think this makes things to be clearer: 9c26e9a.

Additionally including mypy in the testing loop to ensure typing is correct.

@guyonvarch guyonvarch added reviewable Ready for initial or iterative review and removed details needed Further information requested to better evaluate changes labels Nov 13, 2023
@guyonvarch guyonvarch requested review from knuton and stoeffel and removed request for knuton November 13, 2023 16:11
@guyonvarch guyonvarch changed the title Document connman service and add initial tests for the Kiosk browser Add initial tests and typings for the Kiosk browser proxy Nov 16, 2023
stoeffel
stoeffel approved these changes Nov 16, 2023
View reviewed changes
Copy link
Contributor

@stoeffel stoeffel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@stoeffel stoeffel merged commit 399356b into dividat:develop Nov 16, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knuton knuton knuton left review comments

@stoeffel stoeffel stoeffel approved these changes

Assignees
No one assigned
Labels
reviewable Ready for initial or iterative review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@guyonvarch @knuton @stoeffel