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

PR: Improve usability of runtests locally and test output on the CIs #8123

Merged
merged 6 commits into from
Nov 1, 2018

Conversation

CAM-Gerlach
Copy link
Member

Pull Request Checklist

  • Read and followed this repo's Contributing Guidelines
  • Based this PR on the latest version of the correct branch (master or 3.x)
  • Followed PEP 8 code style
  • Ensured this pull request hasn't eliminated unrelated blank lines/spaces,
    modified the spyder/defaults directory, or added new icons/assets
  • Wrote at least one-line docstrings following PEP 257 for any new functions
  • Added at least one unit test covering the changes, if at all possible
  • Described the changes and the motivation for them below

Developer Certificate of Origin Affirmation

By submitting this Pull Request or typing my name below, I affirm the
Developer Certificate of Origin
with respect to both the content of the contribution itself and this post,
and understand I am releasing it under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: C.A.M. Gerlach

Description of Changes

This PR does a few things to improve usability of both runtests locally (that have been bothering me and perhaps others for quite some time), as well as the CI reports for users developing both Spyder 3 and Spyder 4, and does a little opportunistic cleanup/refacotring of code that hasn't been touched between 3.x and master.

Namely, it:

  • Adds the ability to pass any arbitrary additional pytest options of the users choosing through to pytest with argparse, as well as some user-friendly help text and error handling
  • Makes the -x option the default only on the CIs, so local users can see full test output without hacking runtests.py and can still pass -x or Ctrl-C the test cycle manually if they so desire
  • Removes the remaining % file coverage by default, as it can easily be re-enabled locally in the case the user really desires it for every file, and is already displayed in far more accessible form via two different dedicated services when run the CIs and otherwise just adds extra clutter to the logs
  • Does some minor opportunistic cleanup/refactoring of non-conflicting 3.x/master code

It was made against 3.xsince both branches are still being actively developed, and thus the local and CI test output and runtests API would be inconsistent between them and have extra clutter. There should only be one small conflict—the Spyder profiler coverage stuff was removed, which was already removed in master).

@pep8speaks
Copy link

Hello @CAM-Gerlach! Thanks for submitting the PR.

@ccordoba12
Copy link
Member

the Spyder profiler coverage stuff was removed, which was already removed in master

It was removed in master because all plugins are now inside spyder.plugins, but that's not the case for our 3.x branch because spyder_profiler is a package on its own and it lives out of the spyder one. So please restore coverage for it.

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

@CAM-Gerlach, thanks for this. I only hope to not have big arguments about this because I don't have time for them.

runtests.py Outdated Show resolved Hide resolved
runtests.py Outdated Show resolved Hide resolved
runtests.py Show resolved Hide resolved
runtests.py Outdated Show resolved Hide resolved
runtests.py Outdated Show resolved Hide resolved
@CAM-Gerlach
Copy link
Member Author

CAM-Gerlach commented Oct 23, 2018

It was removed in master because all plugins are now inside spyder.plugins, but that's not the case for our 3.x branch because spyder_profiler is a package on its own and it lives out of the spyder one.

Yes, I'm aware :)

So please restore coverage for it.

Perhaps you misunderstand the intent of the relevant change. Removing the cov= options for the profiler plugin is simply to be consistent with

Removes the remaining % file coverage by default, as it can easily be re-enabled locally in the case the user really desires it for every file, and is already displayed in far more accessible form via two different dedicated services when run the CIs and otherwise just adds extra clutter to the logs

It wouldn't make any sense to keep the coverage percentages for just the profiler while not having them for the rest of Spyder. However, if you have a rationale for why retaining them for Spyder as a whole in every CI report is necessary when we already have CodeCov, Coveralls and running it locally, then they would of course be retained as well.

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

I left some more comments. The difference between RUN_CI and run_slow is still confusing to me (as proposed in this PR).

runtests.py Outdated Show resolved Hide resolved
runtests.py Show resolved Hide resolved
runtests.py Outdated Show resolved Hide resolved
@ccordoba12 ccordoba12 modified the milestones: v3.3.2, v3.3.3 Oct 30, 2018
@CAM-Gerlach
Copy link
Member Author

The difference between RUN_CI and run_slow is still confusing to me (as proposed in this PR).

Right, its understandably confusing since there is no difference in 3.x, while there is on master since the run_slow tests are skipped when running on the CI under Darwin which run_slow currently checks for. I just left it as is to make your life easier when merging, but on request I removed it and you'll need to check for it somewhere when you merge with master.

@ccordoba12
Copy link
Member

Thanks for the change @CAM-Gerlach! It looks much better now!

However, please notice that without the --cov options, Codecov and Coveralls don't report anything in the PR checks. Therefore, I think you need to put them back, but please do it only when RUN_CI == True

@CAM-Gerlach
Copy link
Member Author

However, please notice that without the --cov options, Codecov and Coveralls don't report anything in the PR checks.

Thanks; good catch—I should have checked that. I added --cov=spyder and --cov=spyder-profiler to the if RUN_CI additional arguments list (without the --cov-report=term-missing that produced overly verbose text-mode reports, unless that's needed for Codecov/Coveralls—it was removed in master already so I assume not). I also added the --no-cov-on-fail option, which will avoid printing a full coverage report for failed runs (which can get particularly troublesome due to the 3x repeat runs on fail) and also wasting time and resources calculating coverage for a failed build.

@ccordoba12
Copy link
Member

I added --cov=spyder and --cov=spyder-profiler to the if RUN_CI additional arguments list

Yeah, that's fine 👍

I also added the --no-cov-on-fail option

Even better, I didn't know about that option.

@ccordoba12 ccordoba12 modified the milestones: v3.3.3, v3.3.2 Oct 31, 2018
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks @CAM-Gerlach!

@ccordoba12 ccordoba12 changed the title PR: Improve usability of runtests locally and test output on the CIs, and cleanup code PR: Improve usability of runtests locally and test output on the CIs Nov 1, 2018
@ccordoba12 ccordoba12 merged commit fe82bea into spyder-ide:3.x Nov 1, 2018
ccordoba12 added a commit that referenced this pull request Nov 1, 2018
@CAM-Gerlach CAM-Gerlach deleted the cleanup-test-args branch February 12, 2019 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants