Skip to content

Conversation

@Peque
Copy link
Contributor

@Peque Peque commented Dec 12, 2019

While creating the test for #235, I missed a couple of things about pytest:

  • I had to add extra indentation (tests needed to be inside a class, so extra "boilerplate")
  • Output of the test suite was not very pretty

But specially, I had to write:

self.assertAlmostEqual(rotated[0], normal[0])
self.assertAlmostEqual(rotated[1], normal[1])
self.assertAlmostEqual(rotated[2], normal[2])

Instead of (from pytest import approx):

assert rotated == approx(normal)

The latter is definitely shorter, clearer and more Pythonic. 😊

Also, I think nowadays pytest is the de facto tool. Or that is my impression.

Hope you like the shift.

@jmwright
Copy link
Member

@Peque Thanks for the contributions. The CI systems are complaining about pytest not being installed. I think you'll need to add a pip install line to appveyor.yml and .travis.yml to get those checks to pass.

@Peque
Copy link
Contributor Author

Peque commented Dec 12, 2019

@jmwright You are right! I assumed both Travis and AppVeyor would create the Conda environments with the environment.yaml file.

I will fix that. 👍

@Peque
Copy link
Contributor Author

Peque commented Dec 12, 2019

Had a quick look to try to use conda env create -f environment.yaml in both Travis and AppVeyor but:

  • It seems you cannot conda env create -f environment python=other_version (Conda ignores the python=other_version part)
  • Did not find an easy way to edit the config file with AppVeyor (with Travis it is easy, using sed to replace the Python version before creating the virtual environment)

@codecov
Copy link

codecov bot commented Dec 12, 2019

Codecov Report

Merging #236 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #236      +/-   ##
==========================================
- Coverage   94.86%   94.84%   -0.02%     
==========================================
  Files          19       18       -1     
  Lines        4189     4173      -16     
==========================================
- Hits         3974     3958      -16     
  Misses        215      215
Impacted Files Coverage Δ
tests/test_selectors.py 100% <ø> (ø)
tests/test_jupyter.py 100% <ø> (ø)
tests/test_importers.py 93.75% <ø> (ø)
tests/test_cadquery.py 98.97% <ø> (ø)
tests/test_exporters.py 100% <ø> (ø)
tests/test_workplanes.py 100% <ø> (ø)
tests/test_cad_objects.py 99.33% <ø> (ø)
tests/test_cqgi.py 100% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update acf363d...5069483. Read the comment docs.

@Peque
Copy link
Contributor Author

Peque commented Dec 12, 2019

@jmwright Passing now.

Isn't the output prettier this way? 😍 [before vs. after]

@adam-urbanczyk
Copy link
Member

Thanks for the contribution @Peque . Py-test is definitely nicer. Are there any actions required regarding appveyor? BTW there are ways to do sed-like stuff with PowerShell or CMake.

@Peque
Copy link
Contributor Author

Peque commented Dec 12, 2019

@adam-urbanczyk What do you mean by "any actions required regarding appveyor"? I already updated the configuration file (and pipelines are passing).

Yeah, I guessed there would be ways, but I was not willing to spend time on that. 😇

I created this issue, which I think would be the most convenient way to do it: conda/conda#9506

@jmwright
Copy link
Member

@Peque Looks much nicer, thanks. That will be a nice quality of life improvement for all CadQuery contributors.

@adam-urbanczyk I'm all for merging when you're ready.

@adam-urbanczyk
Copy link
Member

@Peque ok, clear. I just wasn't sure if you were done with it. Thanks for the contribution!

@adam-urbanczyk adam-urbanczyk merged commit 5c47899 into CadQuery:master Dec 13, 2019
@Peque Peque deleted the ci branch December 15, 2019 16:53
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.

3 participants