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

Add live progress style and refactor output and progress styles #233

Merged
merged 18 commits into from Jun 5, 2021
Merged

Add live progress style and refactor output and progress styles #233

merged 18 commits into from Jun 5, 2021

Conversation

JoshKarpel
Copy link
Contributor

Putting this up for some early feedback. Have not tested all the corner cases of #205 yet.

@darrenburns I did what I described in #227 and it works great! I think I came up with a fairly clean way of abstracting away the actual test running while giving the styles a fair bit of control over what the output looks like. I've got the "live" output style you describe in the issue, plus a "none" output style that prints nothing, PLUS dots work with progress bars! Some of these are even a little faster in wall clock time because more of the printing is happening on a background thread.

... but none of them will work in CI except test-per-line output with inline progress, because the others all use Lives. In fact, since I'm always starting the live right now, it may mess up CI even with the components that would use it turned off....

@codecov
Copy link

codecov bot commented May 25, 2021

Codecov Report

Merging #233 (cb6a826) into master (b840a94) will increase coverage by 1.27%.
The diff coverage is 50.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #233      +/-   ##
==========================================
+ Coverage   82.18%   83.45%   +1.27%     
==========================================
  Files          21       21              
  Lines        1695     1741      +46     
  Branches      268      270       +2     
==========================================
+ Hits         1393     1453      +60     
+ Misses        259      243      -16     
- Partials       43       45       +2     
Impacted Files Coverage Δ
ward/_debug.py 62.79% <0.00%> (-4.71%) ⬇️
ward/_terminal.py 59.03% <49.68%> (+6.75%) ⬆️
ward/_run.py 86.23% <100.00%> (-0.13%) ⬇️
ward/testing.py 96.51% <100.00%> (+1.03%) ⬆️

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 b840a94...cb6a826. Read the comment docs.

@darrenburns
Copy link
Owner

Looks really cool! Big fan of how you've refactored the output styles code too, it feels much cleaner and seems like it'll make it much easier to offer a hook for plugins to offer custom output styles 🙂

From a UX point of view, I think we'd need a way of seeing if any tests have failed so far (maybe having the little "Results" panel that displays after a test run always be visible and live updating? - not sure if that's trying to stretch the Live API too far, although I haven't looked enough at it)

An issue though, that I suspect may throw a huge spanner in the works here but would love to be wrong... is that it breaks debugging. It seems like the Live continues trying to update the terminal while you're inside pdb 😬 We would probably need a method of suspending running Live inside this function. Whether that's even possible since it'd be outside the context of the context manager 🤷

@JoshKarpel
Copy link
Contributor Author

JoshKarpel commented May 25, 2021

From a UX point of view, I think we'd need a way of seeing if any tests have failed so far (maybe having the little "Results" panel that displays after a test run always be visible and live updating? - not sure if that's trying to stretch the Live API too far, although I haven't looked enough at it)

I was actually thinking of something similar - the failed tests could get printed, so they would be "left behind" by the single updating line. I just pushed this up for you to take a look.

An issue though, that I suspect may throw a huge spanner in the works here but would love to be wrong... is that it breaks debugging. It seems like the Live continues trying to update the terminal while you're inside pdb 😬 We would probably need a method of suspending running Live inside this function. Whether that's even possible since it'd be outside the context of the context manager 🤷

I actually did exactly this for Spiel so that I could drop into a terminal session https://github.com/JoshKarpel/spiel/blob/b37497490dd12e693d481e0c5d81a797c7b922c5/spiel/input.py#L386

As long as you can get a reference to the Live, you're good to go. I'll see if I can figure out a sane way to get a reference over there...

Edit: actually it might be trivial (if not exactly beautiful) because the console in _terminal.py is a global.

@JoshKarpel
Copy link
Contributor Author

Bah, I over-promised. Turns out you can't really wrap the breakpoint call in a context manager. I went with the next-easiest option, which is to disable the Live when entering the breakpoint and unconditionally make sure it's running after each test finishes. Kind of weird behavior if you n off the end of the test in the debugger all the way into Ward's internals.

@JoshKarpel JoshKarpel changed the title Add live progress style Add live progress style and refactor output and progress styles May 30, 2021
@JoshKarpel JoshKarpel marked this pull request as ready for review May 30, 2021 03:50
@JoshKarpel
Copy link
Contributor Author

JoshKarpel commented May 30, 2021

Ok, I think this is ready for a round of closer review.

@darrenburns despite the refactor, I'm still not really sure how to test these in a clean way. I really like how the classes manage their own state, but they're big bags of complicated state now, and it's hard to drive them in a consistent way without executing a test suite... which you can't do because the test suite itself is running a Live and you can't have two Lives running at once. I'm tempted to defer intensive testing of the internals of this once again, and hope that in some other PR we can build a way to test these that is robust and simple enough to not need continual tweaking as the terminal code evolves.

Oh, and

it'll make it much easier to offer a hook for plugins to offer custom output styles 🙂

I think we'll have to ditch the enumerated styles to make this work, if it involves adding new widget classes and registering them somewhere :(

@darrenburns
Copy link
Owner

@JoshKarpel I'll have a look at this soon, hopefully within the next couple of days.

I think we'll have to ditch the enumerated styles to make this work, if it involves adding new widget classes and registering them somewhere :(

That's likely, yeah. There might be a nice __init_subclass__ approach, but will need to think more.

Copy link
Owner

@darrenburns darrenburns left a comment

Choose a reason for hiding this comment

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

Won't be able to look any more for now, but left a couple of comments.

I've got to say in general I absolutely love how clean the code looks now 👏

tests/test_terminal.py Outdated Show resolved Hide resolved
ward/_terminal.py Outdated Show resolved Hide resolved
ward/testing.py Outdated Show resolved Hide resolved
@darrenburns darrenburns merged commit cdcea6a into darrenburns:master Jun 5, 2021
@darrenburns
Copy link
Owner

This is great, thanks! ❤️

@JoshKarpel JoshKarpel deleted the live branch June 5, 2021 15:06
mkuyper added a commit to mkuyper/ward that referenced this pull request Aug 19, 2021
* master: (133 commits)
  Prepare 0.63.0b0
  Distribute type data (PEP 561) (darrenburns#283)
  Type check `ward.testing` (darrenburns#282)
  [pre-commit.ci] pre-commit autoupdate (darrenburns#280)
  Add pretty output for all comparison failures (darrenburns#256)
  Update conf.py
  Update pyproject.toml
  Make sure raises raises assertion error when no exception is raised (darrenburns#281)
  [pre-commit.ci] pre-commit autoupdate (darrenburns#275)
  Fix mypy error
  Prepare 0.62.0b0
  Update writing_tests.rst
  Allow subclasses of specified exception class to pass raises assertion (darrenburns#279)
  Type check `ward.expect`, `ward._fixtures` and `ward._terminal` (darrenburns#274)
  Only require `poetry-core` as build system (darrenburns#277)
  Prepare 0.61.1b0
  Allow Click 7 (darrenburns#272)
  Type check `ward._config` (darrenburns#269)
  Correct plural, singular forms darrenburns#244 (darrenburns#258)
  Prepare 0.61.0b0
  Switch from `toml` to `tomli` for TOML v1 compat (darrenburns#267)
  Introduce mypy (gradually) (darrenburns#265)
  Don't update the lockfile with 'make prep' (darrenburns#266)
  Let Poetry automatically update license and py version classifiers (darrenburns#260)
  Prepare 0.60.1b0
  Fix broken command  (darrenburns#257)
  fix typo (darrenburns#255)
  Prepare 0.60.0b0
  Update live output gif for docs
  Add info on `live` output mode to docs
  Print pretty failure messages for `in` and `not in` (darrenburns#242)
  Add `live` progress style and refactor output and progress styles (darrenburns#233)
  only get test source once (darrenburns#249)
  [pre-commit.ci] pre-commit autoupdate (darrenburns#248)
  pygments dependency removed - no longer required
  Update build.yml
  Update README.md
  Prepare 0.59.0b0
  Use Rich for displaying diffs in errors (darrenburns#235)
  Prepare 0.58.0b0
  Several bug fixes (darrenburns#241)
  combine build and pullrequest workflows (darrenburns#236)
  Add Python 3.10 Beta support to CI (darrenburns#230)
  Update CONTRIBUTING.md
  Prepare 0.57.2b0
  Remove comment from build pipeline
  Bump snok/install-poetry from 1.1.1 to 1.1.6 (darrenburns#232)
  add dependabot.yml for gha version bumps (darrenburns#231)
  Add pre-commit (darrenburns#222)
  fix linter errors (darrenburns#217)
  ...
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.

None yet

2 participants