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

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

Merged
merged 4 commits into from
Oct 20, 2023
Merged

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

merged 4 commits into from
Oct 20, 2023

Conversation

Inokinoki
Copy link
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

linear bot commented Aug 1, 2023

GSK-1508 Add unit tests for WebSocket communication DTOs

@github-actions
Copy link

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
Copy link
Contributor

@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
Copy link
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
Member Author

@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
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.

python-client/tests/communications/test_dto_parsing.py Outdated Show resolved Hide resolved
python-client/tests/communications/test_dto_parsing.py Outdated Show resolved Hide resolved
python-client/tests/communications/test_dto_parsing.py Outdated Show resolved Hide resolved
python-client/tests/communications/test_dto_parsing.py Outdated Show resolved Hide resolved
python-client/tests/communications/test_dto_parsing.py Outdated Show resolved Hide resolved
python-client/tests/communications/test_dto_serialization.py Outdated Show resolved Hide resolved
python-client/tests/communications/test_dto_serialization.py Outdated Show resolved Hide resolved
python-client/tests/communications/test_dto_serialization.py Outdated Show resolved Hide resolved
python-client/tests/communications/test_dto_serialization.py Outdated Show resolved Hide resolved
python-client/tests/communications/test_dto_serialization.py Outdated Show resolved Hide resolved
@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
@Inokinoki
Copy link
Member Author

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

@sonarcloud
Copy link

sonarcloud 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
13 checks passed
@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

@andreybavt andreybavt andreybavt left review comments

@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

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