Skip to content

[GSK-1508] Add unit tests for WebSocket-based ML Worker communication (python)#1277

Merged
Hartorn merged 4 commits intomainfrom
websocket-unit-tests
Oct 20, 2023
Merged

[GSK-1508] Add unit tests for WebSocket-based ML Worker communication (python)#1277
Hartorn merged 4 commits intomainfrom
websocket-unit-tests

Conversation

@Inokinoki
Copy link
Copy Markdown
Member

@Inokinoki Inokinoki commented Aug 1, 2023 •
edited
Loading

Description

Related Issue

Type of Change

  • 📚 Examples / docs / tutorials / dependencies update
  • 🔧 Bug fix (non-breaking change which fixes an issue)
  • 🥂 Improvement (non-breaking change which improves an existing feature)
  • 🚀 New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to change)
  • 🔐 Security fix

Checklist

  • I've read the CODE_OF_CONDUCT.md document.
  • I've read the CONTRIBUTING.md guide.
  • I've updated the code style using make codestyle.
  • I've written tests for all new methods and classes that I created.
  • I've written the docstring in Google format for all the methods and classes that I used.

@Inokinoki Inokinoki changed the title Add unit tests for WebSocket-based ML Worker communication [GSK-1508] Add unit tests for WebSocket-based ML Worker communication Aug 1, 2023
@linear
Copy link
Copy Markdown

linear Bot commented Aug 1, 2023

GSK-1508 Add unit tests for WebSocket communication DTOs

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Aug 1, 2023

Please add the 'safe for build' label in order to perform the sonar analysis!

@Inokinoki Inokinoki mentioned this pull request Aug 1, 2023
Merged
11 tasks
@Inokinoki Inokinoki changed the title [GSK-1508] Add unit tests for WebSocket-based ML Worker communication [Draft][GSK-1508] Add unit tests for WebSocket-based ML Worker communication Aug 1, 2023
@Inokinoki Inokinoki added Python Pull requests that update Python code safe for build labels Aug 1, 2023
@Inokinoki Inokinoki changed the title [Draft][GSK-1508] Add unit tests for WebSocket-based ML Worker communication [GSK-1508] Add unit tests for WebSocket-based ML Worker communication Aug 1, 2023
@Inokinoki Inokinoki mentioned this pull request Aug 30, 2023
Merged
andreybavt
andreybavt previously requested changes Sep 8, 2023
View reviewed changes
Comment thread backend/src/test/java/ai/giskard/ml/MLWorkerReplyAggregatorTest.java Outdated
Comment thread backend/src/test/java/ai/giskard/config/WebSocketChannelInterceptorTest.java Outdated
Comment thread python-client/tests/communications/test_listener_utils.py Outdated
andreybavt
andreybavt reviewed Sep 8, 2023
View reviewed changes
Comment thread python-client/tests/communications/test_dto_parsing.py Outdated
andreybavt
andreybavt reviewed Sep 8, 2023
View reviewed changes
Comment thread python-client/tests/communications/test_dto_serialization.py Outdated
@andreybavt
Copy link
Copy Markdown
Contributor

andreybavt commented Sep 14, 2023

@Inokinoki could you check the comments and refresh this PR please?

@Inokinoki Inokinoki self-assigned this Sep 15, 2023
@Inokinoki

This comment was marked as outdated.

Sign in to view
andreybavt
andreybavt reviewed Sep 22, 2023
View reviewed changes
Comment thread python-client/giskard/ml_worker/websocket/utils.py Outdated
andreybavt
andreybavt reviewed Sep 22, 2023
View reviewed changes
Comment thread python-client/tests/communications/test_listener_utils.py Outdated
andreybavt
andreybavt reviewed Sep 22, 2023
View reviewed changes
Comment thread python-client/tests/communications/test_dto_parsing.py Outdated
andreybavt
andreybavt reviewed Sep 22, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@andreybavt andreybavt left a comment

Choose a reason for hiding this comment

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

General comment: Can you reduce code duplication in tests? It makes them harder to maintain in the future.

To have an inspiration about how tests could be structured you can refer to Uncle Bob's "Clean Code" that gives a few good advice:

https://blog.cleancoder.com/uncle-bob/2013/09/23/Test-first.html

the main points are:

  • test code quality criterias shouldn't be different from production code (duplication isn't appreciated in either of them)
  • tests should be small and focused

@Inokinoki
Copy link
Copy Markdown
Member Author

Inokinoki commented Sep 26, 2023

@Googleton I also created a bunch of tests for DTOs in push feature. Could you please review them and tell me if I understand well?

Hartorn
Hartorn requested changes Sep 27, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@Hartorn Hartorn left a comment

Choose a reason for hiding this comment

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

I feel like you made too many tests :p
What I mean is the following : tests are done to ensure our code is functioning

Adding one test to make sure some lib is properly working is fine, but extensively testing validation for every pydantic object is too much : we have to assume that the libs we use are working as expected.

Comment thread python-client/tests/communications/test_dto_parsing.py Outdated
Comment thread python-client/tests/communications/test_dto_parsing.py Outdated
Comment thread python-client/tests/communications/test_dto_parsing.py Outdated
Comment thread python-client/tests/communications/test_dto_parsing.py Outdated
Comment thread python-client/tests/communications/test_dto_parsing.py Outdated
Comment thread python-client/tests/communications/test_dto_serialization.py Outdated
Comment thread python-client/tests/communications/test_dto_serialization.py Outdated
Comment thread python-client/tests/communications/test_dto_serialization.py Outdated
Comment thread python-client/tests/communications/test_dto_serialization.py Outdated
Comment thread python-client/tests/communications/test_dto_serialization.py Outdated
@Hartorn Hartorn marked this pull request as draft September 28, 2023 16:20
@Hartorn Hartorn force-pushed the websocket-unit-tests branch 3 times, most recently from 1636e35 to 5adda4f Compare September 29, 2023 11:49
@Hartorn Hartorn force-pushed the websocket-unit-tests branch 3 times, most recently from c5373ed to 20ba20d Compare October 2, 2023 08:02
@Hartorn Hartorn marked this pull request as ready for review October 2, 2023 08:02
@Hartorn Hartorn force-pushed the websocket-unit-tests branch 2 times, most recently from 57fe93d to c90696c Compare October 2, 2023 12:05
@Hartorn Hartorn changed the title [GSK-1508] Add unit tests for WebSocket-based ML Worker communication [GSK-1508] Add unit tests for WebSocket-based ML Worker communication (python) Oct 2, 2023
@Hartorn Hartorn force-pushed the websocket-unit-tests branch from c90696c to e80ca29 Compare October 9, 2023 08:18
@Inokinoki
Copy link
Copy Markdown
Member Author

Inokinoki commented Oct 12, 2023

Thanks very much for greatly improving this PR @Hartorn !
This is much clearer and easier to maintain.
LGTM

@Hartorn Hartorn force-pushed the websocket-unit-tests branch from 5dabeb5 to 1471b38 Compare October 12, 2023 11:20
@Inokinoki Inokinoki requested a review from Hartorn October 13, 2023 08:24
@Hartorn Hartorn force-pushed the websocket-unit-tests branch from f709d1d to 3bed1e1 Compare October 18, 2023 18:17
@Hartorn Hartorn force-pushed the websocket-unit-tests branch from 3bed1e1 to 0402fc7 Compare October 19, 2023 22:16
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Oct 20, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@Hartorn Hartorn dismissed andreybavt’s stale review October 20, 2023 12:32

Outdated comments :)

@Hartorn Hartorn merged commit d10b897 into main Oct 20, 2023
@Hartorn Hartorn deleted the websocket-unit-tests branch October 20, 2023 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Hartorn Hartorn Hartorn approved these changes

@kevinmessiaen kevinmessiaen Awaiting requested review from kevinmessiaen

@rabah-khalek rabah-khalek Awaiting requested review from rabah-khalek

@Googleton Googleton Awaiting requested review from Googleton

+1 more reviewer

@andreybavt andreybavt andreybavt left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@Inokinoki Inokinoki

Labels

Python Pull requests that update Python code safe for build

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Inokinoki @andreybavt @Hartorn