radish-bdd / radish

Behavior Driven Development tooling for Python. The root from red to green.
https://radish-bdd.github.io
MIT License
181 stars 49 forks source link

Migrate from Colorful to rich #465

Closed inquisitev closed 1 month ago

inquisitev commented 1 year ago

Migrating from Colorful to Rich

I had to set some of the console settings to get a consistent output for tests. That includes the size of the terminal, and the out file to be io.StringIO as recommended by Rich console capture.

Rich doesn't handle the line jump sequence in the same way as colorful, so i keep a list of lines to write to a rich live session, and just use list::pop to remove lines for color changing. Live output is not captured by the console, so when live is off (during tests) i print directly to that console.

codecov[bot] commented 1 year ago

Codecov Report

Attention: Patch coverage is 80.00000% with 33 lines in your changes missing coverage. Please review.

Project coverage is 87.30%. Comparing base (4688b59) to head (945d423). Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
radish/testing/matches.py 41.93% 18 Missing :warning:
radish/printer.py 86.79% 7 Missing :warning:
radish/extensions/formatters/gherkin.py 91.66% 4 Missing :warning:
radish/errororacle.py 60.00% 2 Missing :warning:
radish/extensions/endreport_writer.py 94.11% 1 Missing :warning:
radish/main.py 66.66% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #465 +/- ## ========================================== + Coverage 87.26% 87.30% +0.04% ========================================== Files 39 40 +1 Lines 2380 2427 +47 ========================================== + Hits 2077 2119 +42 - Misses 303 308 +5 ``` | [Flag](https://app.codecov.io/gh/radish-bdd/radish/pull/465/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=radish-bdd) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/radish-bdd/radish/pull/465/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=radish-bdd) | `87.30% <80.00%> (+0.04%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=radish-bdd#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

inquisitev commented 1 year ago

@fliiiix, would you be interested in a CI step that will publish the actual output for each integration test on failure?

this would be convenient for me, as i develop on my mac

I would add something like this

      - uses: actions/upload-artifact@v3
        if: failure()
        with:
          name: expected_results
          path: path/to/artifact/

under the integration test step, and add an environment variable like STASH_ACTUAL_OUTPUT_ON_FAILURE=1 to enable this functionality.

in test_main, i could check that env var and write the output directly to the expected output, but still return the failed status.

thoughts?

fliiiix commented 1 year ago

Sorry i missed your question here, i mean just put it all in a single commit and as long as it is not to complex to maintain and understand im all for such a functionality so if it helps you do it and then we can take a look at the implementation if we can also merge it to master :+1:

inquisitev commented 1 year ago

@fliiiix, i think this is ready for a review.

Codecov is failing because some of those lines that were changed (colorful to rich) are not covered by tests.

There is also some console settings that are not covered by tests, but i dont think it makes sense to do that because the only time that behavior shows is when truly running

fliiiix commented 1 year ago

Nice :+1: Also i don't care much about codecove its more an indication if we forgot something to test, but i couldn't figure out how to not make that fail

I will take a look at the change

inquisitev commented 10 months ago

@fliiiix this has been open for quite a while, can you take a look?

fliiiix commented 10 months ago

Sorry this is still on my todo list, the upstream project using Python 3.5 should have been migrated last year but is obviously delayed, but at least the process has started so im still hopeful to make this happening Q1 2024.

I had a quick review and the code looks fine, i still will do some manual sanity checks by hand to see if everything still looks correct on unix and windows.

inquisitev commented 1 month ago

Feel free to reopen this PR when you are ready for it