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

Operation refactor (part 1) #37

Merged
merged 3 commits into from
May 8, 2023

Conversation

giannissc
Copy link
Contributor

I am introducing Tool and Spindle changes to the Fluent API and all the necessary tests

In Part 2 I will be introducing Feed changes. This is going to be messier so I opted for breaking them into 2 PR.

@giannissc giannissc requested a review from voneiden as a code owner May 6, 2023 17:23
@codecov
Copy link

codecov bot commented May 6, 2023

Codecov Report

Merging #37 (1c3c53e) into unstable (4cd6fa6) will increase coverage by 1.36%.
The diff coverage is 99.24%.

❗ Current head 1c3c53e differs from pull request most recent head 1cb1784. Consider uploading reports for the commit 1cb1784 to get more accurate results

@@             Coverage Diff              @@
##           unstable      #37      +/-   ##
============================================
+ Coverage     81.76%   83.13%   +1.36%     
============================================
  Files            34       36       +2     
  Lines          2462     2567     +105     
============================================
+ Hits           2013     2134     +121     
+ Misses          449      433      -16     
Impacted Files Coverage Δ
src/cq_cam/common.py 100.00% <ø> (ø)
src/cq_cam/operations/base_operation.py 78.33% <ø> (ø)
src/cq_cam/fluent.py 87.19% <96.42%> (+3.13%) ⬆️
src/cq_cam/command.py 86.72% <100.00%> (+0.19%) ⬆️
src/cq_cam/tests/conftest.py 100.00% <100.00%> (ø)
src/cq_cam/tests/test_drill.py 100.00% <100.00%> (ø)
src/cq_cam/tests/test_gcode.py 100.00% <100.00%> (ø)
src/cq_cam/tests/test_pocket.py 100.00% <100.00%> (ø)
src/cq_cam/tests/test_profile.py 100.00% <100.00%> (ø)
src/cq_cam/tests/test_tool_change.py 100.00% <100.00%> (ø)
... and 1 more

... and 3 files with indirect coverage changes

@giannissc giannissc force-pushed the operation-refactor branch 2 times, most recently from 5214169 to 67b22c8 Compare May 6, 2023 18:16
@giannissc
Copy link
Contributor Author

SonarCloud is raising duplication errors on the test strings. Should I try to fix? @voneiden

@voneiden
Copy link
Owner

voneiden commented May 6, 2023

The duplication errors are for the four last tests which share a very similar structure. If you want to get your hands dirty, check out parametrization @ https://docs.pytest.org/en/6.2.x/parametrize.html

You could parametrize the tool(s) and expected outputs and have a single test.

The test string code smell nags are because there are no line breaks but the strings are broken into multiple string sections, like

x = ("foo" "bar")

vs

x = ("foo"
     "bar")

It's also not necessary to always test full gcode output - in fact testing only within the scope of the test would be preferable in this case. Makes the test less fragile to noise.

@giannissc
Copy link
Contributor Author

The test string is modified by black so If I add the line then black check will fail

@giannissc
Copy link
Contributor Author

Can I parametrize across test files. test_pocket, test_profile and test_drill they same the exact same thing. The operation that happens is irrelevant since we are just checking what gets added between operation. Where should I include such a test? A new test_tool_change file? @voneiden

@voneiden
Copy link
Owner

voneiden commented May 8, 2023

My bad. Maybe that can be resolved by adding a comment on the first line, then black shouldn't be able to touch the line breaks:

x = (
    # black
    "foo"
    "bar"
)

L

@voneiden
Copy link
Owner

voneiden commented May 8, 2023

New file is fine and perhaps the operation can be one parameter of the test.

@giannissc giannissc force-pushed the operation-refactor branch 4 times, most recently from 0e31ea4 to dfd86e2 Compare May 8, 2023 12:31
@sonarcloud
Copy link

sonarcloud bot commented May 8, 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

No Coverage information No Coverage information
0.0% 0.0% Duplication

@sonarcloud
Copy link

sonarcloud bot commented May 8, 2023

Please retry analysis of this Pull-Request directly on SonarCloud.

@giannissc
Copy link
Contributor Author

Ready! @voneiden

Copy link
Owner

@voneiden voneiden 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, thank you.

@voneiden voneiden merged commit def5f37 into voneiden:unstable May 8, 2023
This was referenced May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants