Coverage & CI Setup Improvement by vmuriart · Pull Request #335

Coverage & CI Setup Improvement by vmuriart · Pull Request #335 · pythonnet/pythonnet

What does this implement/fix? Explain your changes.

Add coverage (cs code limited to windows, no mature coverage tools for mono)
Cleans up CI configurations
Speeds up Travis ~30% by using containers
Simplifies and unfreezes mono setup
Speed up AppVeyor by 50% by not downloading and building with miniconda.

Does this close any currently open issues?

#237
#334

Any other comments?

Sample coverage report.

Checklist

Check all those that are applicable and complete.

Updated the CHANGELOG

Codecov Report

❗ No coverage uploaded for pull request base (master@ef133a3).

@@           Coverage Diff            @@
##             master    #335   +/-   ##
========================================
  Coverage          ?   61.1%           
========================================
  Files             ?      61           
  Lines             ?    5324           
  Branches          ?     897           
========================================
  Hits              ?    3253           
  Misses            ?    1832           
  Partials          ?     239

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 ef133a3...44ba698. Read the comment docs.

		- sudo DEBIAN_FRONTEND=noninteractive apt-get -y -o Dpkg::Options::="--force-confdef" -o Dpkg::Options::="--force-confnew" install mono-devel mono-complete referenceassemblies-pcl ca-certificates-mono nunit-console
		env:
		global:
		- LD_PRELOAD=/lib/x86_64-linux-gnu/libSegFault.so

Choose a reason for hiding this comment

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

can you explain the purpose of this?

Choose a reason for hiding this comment

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

@vmuriart this pull request removes the conda packages. Is there any reason for this?

@vmuriart can we break this into 2 PR? one for code coverage and one for build improvements?

The conda packaging is what makes the builds the AppVeyor extremely slow and we don't even use them for testing. Do we need them as part of the CI?

@vmuriart we build conda packages, and provide the recipe and downloadable binaries (from appveyor artifacts) for everyone to use in this project. @filmor was very happy about conda support. we do not test conda package, but could add couple lines in appveyor.yml to do this.

Regarding the low speed of the builds - we can re-use installed Miniconda environments from appveyor CI.

It is also important to show how the conda recipe is used to build the package, since this is not trivial.

Finally I plan to extend this conda recipe to conda-forge.

I tried using the built in miniconda's, they were incomplete (don't have the build command 😞.

I thought the interest on the original pr was to create the recipe for miniconda (which is still with the package). @filmor can clarify a bit more on this.

If the intent though really is for AppVeyor to build the miniconda packages, we can have them build only on pr or when the build was triggered by a schedule (ie, they would be built once a day)

You are correct on the original intent of the conda recipe. But if you follow the conversation, then it is obvious that without showing the actual build setup, it is extremely hard to get it right. conda build is installed like this: `conda install conda-build` I'm ok with automated nightly builds for conda (no user intervention) or with migrating the builds to conda-forge. But not removing the conda builds.

…

On Sun, Jan 29, 2017 at 12:34 AM, Victor Uriarte ***@***.***> wrote: I tried using the built in miniconda's, they were incomplete (don't have the build command 😞. I thought the interest on the original pr was to create the recipe for miniconda (which is still with the package). @filmor <https://github.com/filmor> can clarify a bit more on this. If the intent though really is for AppVeyor to build the miniconda packages, we can have them build only on pr or when the build was triggered by a schedule (ie, they would be built once a day) — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#335 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHgZ5aFbki7NAKYpO2VvWrq-winYn_hSks5rXDMRgaJpZM4Lvi59> .

Long-term, moving this to conda-forge is probably the better way. For now I rewrote the pull request to keep conda-build part of the CI, but only build them if its part of a Pull Request.

On my fork they weren't building, but it did once it was uploaded to this pull request.
It makes more sense to build on pull requests than on a scheduled basis, since the scheduled one will probably keep rebuilding the same unchanged branch.

whoops, deleted the wrong branch earlier. sorry

Choose a reason for hiding this comment

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

In general, I think we should have most if not all testing integrated into the conda build recipe. That could simplify the travis and appveyor configuration quite a bit

		- PYTHON_VERSION: 3.3
		- PYTHON_VERSION: 3.4
		- PYTHON_VERSION: 3.5
		- PYTHON_VERSION: 3.6

Choose a reason for hiding this comment

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

Are you sure about this? Wouldn't this result in the version numbers being parsed as floats?

Choose a reason for hiding this comment

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

They are still being used as strings. Remeber this is doing set PYTHON_VERSION=3.3 which comes out as a string when you do %PYTHON_VERSION%

		- set PYTHON=C:\PYTHON%PYTHON_VERSION:.=%
		- if %PLATFORM%==x86 (set CONDA_BLD_ARCH=32)
		- if %PLATFORM%==x86 (set NUNIT=%NUNIT%-x86)
		- if %PLATFORM%==x64 (set PYTHON=%PYTHON%-x64)

Choose a reason for hiding this comment

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

Why don't you use %CONDA_PY%? Why change the casing?

Choose a reason for hiding this comment

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

%CONDA_PY% was removed originally, I added it back later, but decided to keep it on PYTHON_VERSION in case it was removed later.
The casing was to keep all constants on upper case and the rest on lowercase.

		# Shortcut path to executables. Mostly because of OpenCover
		- set PYTHON_EXE=%PYTHON%\python.exe
		- set NUNIT_EXE=.\packages\NUnit.Runners.2.6.2\tools\%NUNIT%.exe
		- set OPENCOVER_EXE=.\packages\OpenCover.4.6.519\tools\OpenCover.Console.exe

Choose a reason for hiding this comment

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

You are hard-coding versions here, this just leads to unnecessary pain (e.g. I'd like to update NUnit in the future).

Choose a reason for hiding this comment

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

Its a very annoying shortcoming from OpenCover.

		test_script:
		- ps: '& "$env:PYTHON\\Scripts\\pip.exe" install --no-cache-dir --force-reinstall --ignore-installed ("dist\\" + (gci dist\*.whl)[0].Name)'
		- ps: copy-item (gci -path build -re -include Python.Test.dll)[0].FullName C:\testdir
		- "%PYTHON%\\python.exe src\\tests\\runtests.py"

Choose a reason for hiding this comment

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

Is this still carried out somewhere?

Choose a reason for hiding this comment

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

@filmor My problem with that is that the conda build adds an extra 60-90 seconds per build and then it wouldnt be testing under our standard packing that we would use on pypi. If you unpack the wheel and the conda recipe output, they are nearly the same so double testing/building is redundant with the exception that conda build puts alot of it out of our hands.

Originally I wanted to remove the conda build altogether from the CI testing which simplifies this alot more, but was talked into leaving it and made it so that it builds once a pull_request has been submitted.

+ Unfreezes mono
+ Moves nunit runner to packages.config