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

Run tests and linters on multiple python versions and on linux #100

Merged
merged 5 commits into from
Feb 12, 2020

Conversation

perrinjerome
Copy link
Contributor

Description (e.g. "Related to ...", etc.)

  • Run typechecker (mypy) as part of test suite
  • Run on multiple python versions
  • Run on linux

Fixes #96

Code review checklist (for code reviewer to complete)

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Commit messages are meaningful (see this for details)
  • Tests have been included and/or updated, as appropriate
  • Docstrings have been included and/or updated, as appropriate
  • Standalone docs have been updated accordingly
  • CONTRIBUTORS.md was updated, as appropriate
  • Changelog has been updated, as needed (see CHANGELOG.md)

@perrinjerome
Copy link
Contributor Author

This is work in progress, this also contain the changes from #99 so that mypy test also pass.

This has 8 jobs: 4 python versions * 2 OS versions, maybe we don't need so much and testing for example all python versions on windows and only python 3.8 on linux is enough.

@perrinjerome
Copy link
Contributor Author

This uses a trick from https://andrewlock.net/building-asp-net-core-apps-on-both-windows-and-linux-using-appveyor/#2-update-your-build-script-optional- to run windows only commands with ps: >.

There was some power shell steps to check the example extension, I have not adapted this for linux, this part is only tested on windows.

@perrinjerome perrinjerome force-pushed the feat/appveyor_matrix branch 5 times, most recently from 9e7407d to 2a58172 Compare January 28, 2020 00:58
@bollwyvl
Copy link
Contributor

I'd highly recommend getting off appveyor in favor of azure pipelines (10 concurrent machines) or github actions (20 concurrent machines).

@danixeee
Copy link
Contributor

I forgot that we already have one project in Azure under our organisation, so we could switch this one as well. @dgreisen Thoughts?

@danixeee danixeee self-requested a review January 28, 2020 10:49
@danixeee danixeee added devops Infrastructure, deployment, etc. enhancement New feature or request labels Jan 28, 2020
@perrinjerome perrinjerome force-pushed the feat/appveyor_matrix branch 2 times, most recently from 8bab07f to 76e0cc9 Compare February 3, 2020 00:24
@perrinjerome
Copy link
Contributor Author

I have updated this, for now to include profiles to run tests on both appveyor and azure-pipelines, but the idea is that we should decide and keep only one.

I have configured my fork of pygls to run on azure and on azure, it looks like this
https://github.com/perrinjerome/pygls/runs/422242124 and for appveyor we can see this on this PR, it's https://ci.appveyor.com/project/oll-team/pygls/builds/30524438 .

Total time on azure was 4m 12s. Total time on appveyor was 5m 49s ... which is strange because it's not the sum of all tests runs. This is the timing reported by both platforms.

Test pass on all python versions for both platforms, but:

  • I had to add a MANIFEST.in for python3.5 compatibility. For more details, python 3.5.9 ( the version appveyor has for ubuntu) was OK, but the problem happened on python 3.5.4 (the version appveyor has for windows) was not. I no longer have traceback, but it was something like README.md not found during install. I have not investigated this further.
  • appveyor pipeline builds a .vsix of the sample json extension ( https://ci.appveyor.com/project/oll-team/pygls/builds/30402659/artifacts ) on appveyor this is only for windows. The steps to build the extension are written in powershell and it did not seem easy to make this work on linux so I did nothing in this area.
  • the azure profiles do not generate artifacts but does the same check of the extension (check that it contain necessary files)

What are your thoughts ?

@danixeee
Copy link
Contributor

danixeee commented Feb 3, 2020

I agree, we should keep only one. I prefer the one which is faster and have better open-source plan.

appveyor pipeline builds a .vsix of the sample json extension ( https://ci.appveyor.com/project/oll-team/pygls/builds/30402659/artifacts ) on appveyor this is only for windows. The steps to build the extension are written in powershell and it did not seem easy to make this work on linux so I did nothing in this area.

I think we could reuse some parts of code from here to build and upload vsix artifact, but that's not urgent since I am building it locally during reviews.

@danixeee
Copy link
Contributor

@perrinjerome Sorry for the delay. We have created project in Azure devops, but I am not able to load yaml file from forked repository's branch.

If everything is ready, please update your branch and create a PR. I will merge both azure and appveyor scripts to the master branch and make some changes after that.

To run lint and tests on all supported python versions
This is required to make a source dist with python setup.py sdist on old
python3.5. Tox includes such a step.
Some steps related to building the sample json extension are only on
windows, because they are implemented in powershell.
This only runs tests and check extensions can be built. On appveyor
there was steps to build/publish the sample json extension, this was not
re-implemented here.
@perrinjerome perrinjerome marked this pull request as ready for review February 11, 2020 04:43
@perrinjerome
Copy link
Contributor Author

@danixeee I updated this branch ( rebased on master ) and marked this PR "Ready for review".

Copy link
Contributor

@danixeee danixeee 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, thanks!

@danixeee danixeee merged commit 3728b42 into openlawlibrary:master Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devops Infrastructure, deployment, etc. enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run test suite on multiple python versions and also on linux
3 participants