jquast / blessed

Blessed is an easy, practical library for making python terminal apps
http://pypi.python.org/pypi/blessed
MIT License
1.2k stars 72 forks source link

Enable GitHub Actions #210

Closed avylove closed 3 years ago

avylove commented 3 years ago

This is a good start, but there are few things to do and discuss.

  1. Upload to codecov currently doesn't work. Error is "Could not determine repo and owner". I think this just means we need to set CODECOV_TOKEN. Can you add this to your GitHub secrets? Secrets are at the user/org level, so I can't. The tokens are repo-specific, so you'd probably want to add it as something like CODECOV_TOKEN_BLESSED. Then we can access it with ${{ secrets.CODECOV_TOKEN_BLESSED }}

  2. Optional test failures (ex: 3.10-dev) cause the commit summary to be a failure (red X instead of green checkmark). This is because all the tests show up individually in the report, rather than a summary like we had with travis-ci. There are some third-party actions that will merge them, but I haven't tried any of them. There is also an option, continue-on-error, that can be added to the tox run step, but then the test will always look successful and you have to look at the logs to see what happened. Anyway, I don't think this is a showstopper, more something to be aware of. The way I configured the tests, it shows up as Tests-optional (3.10-dev, py310,codecov, 1

image

  1. 3.10-dev tests are failing because pytest 6.2.4 is needed. I think to move to that version we need to make some changes to the tox config and maybe other configs. I haven't had a chance to dig into it.
avylove commented 3 years ago

I improved the naming so it's easier to read. You should be able to see the new names below. Since the names can be determined dynamically and the rollup status includes the optional tests, I was able to get rid of some repetition in the config too.

avylove commented 3 years ago

Made some changes

One thing to note, this workflow includes a schedule to run weekly, so we should get notified when a dependency breaks something. I'll fix those as they come along. I feel like it's easier to do that than to wade through a bunch of them every year or two.

Only outstanding item is uploading coverage files. It looks like we can use the codecov-action and public repos aren't supposed to need a token. I might try that in the next day or two. I did notice py34 doesn't work correctly because it requires the old version of Ubuntu and that doesn't have Python 3.8, so, if we use tox -e codecov, that might need a workaround.

codecov[bot] commented 3 years ago

Codecov Report

Merging #210 (a3f5939) into master (0770aa1) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #210   +/-   ##
=======================================
  Coverage   95.44%   95.44%           
=======================================
  Files           9        9           
  Lines        1009     1009           
  Branches      164      164           
=======================================
  Hits          963      963           
  Misses         41       41           
  Partials        5        5           
Impacted Files Coverage Δ
blessed/__init__.py 77.77% <ø> (ø)
blessed/color.py 100.00% <ø> (ø)
blessed/colorspace.py 100.00% <ø> (ø)
blessed/formatters.py 100.00% <ø> (ø)
blessed/keyboard.py 100.00% <ø> (ø)
blessed/terminal.py 98.24% <100.00%> (ø)
blessed/win_terminal.py 33.33% <100.00%> (ø)

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 0770aa1...a3f5939. Read the comment docs.

avylove commented 3 years ago

Was able to get codecov upload to work, but coverage on windows seems to be lower. I'll try to investigate why.

avylove commented 3 years ago

@jquast, Well, the problem with the windows coverage seems to be the codecov uploader. It looks like it includes a bunch of files in the report that it shouldn't, like everything in .tox. I can tell it to not list any files in the report, but then it doesn't know they should be in the blessed directory. We could add a config to tell it that's where they are, but I think using the Python uploader makes more sense for now. So if you populate the CODECOV_TOKEN_BLESSED secret, I can verify.

I did submit a bug for this because they should skip those directories by default, but not sure if and when that will be fixed.

jquast commented 3 years ago

I added as CODECOV_TOKEN not sure why "probably want to add it as something like CODECOV_TOKEN_BLESSED" doesn't seem to matter, at least for their documentation it is explicit on adding a token, anyway I pushed such a change and we will see what happens then

avylove commented 3 years ago

I think each repo has a different codecov token, but you can confirm. So if you want to apply it to wcwidth and other repos, there would be a conflict.

Let me switch it back to the python uploader. The GH actions uploader works without a token, but it doesn't filter out .tox, so terminal.py and __init__.py are getting excluded for Windows builds.

avylove commented 3 years ago

So that worked for windows, but I need to do a little more work. FIrst, because not all environments were uploaded, and second, because the labels are missing to tell what was.

avylove commented 3 years ago

It's looking much better, but still missing coverage in three spots that it had before. I'll try to investigate later to see why.

avylove commented 3 years ago

Another problem was I assumed TEST_QUICK=0 would disable quick tests, but the logic was just looking if it was defined. I changed the logic in the tests to match my assumption. We're a little inconsistent as QUICK_TEST takes 0 or 1 while TEST_RAW and TEST_KEYBOARD are both looking for 'yes'. Do you think we should centralize that logic and coercing anything that's not no, 0, or false to True?

The coverage difference is now down to a single line.

https://github.com/jquast/blessed/blob/master/blessed/formatters.py#L158

From what I can tell, it's because in newer OSes, vpa and hpa are defined for screen and ansi, so they don't generate ParameterizingProxyString. rc, sc, and civis should be, but they won't need to call the string since they don't take parameters. The last tests we ran on travis-ci were on Ubuntu 16.04 vs 20.04 now.

jquast commented 3 years ago

making it uniform is fine, it was created because downstream folks didn't like timing-based tests, esp. in silly slow travis-ci like environments, if i recall? I guess it should be only test_quick=yes by default with =no in ours, or anyway you want really i don't mind

avylove commented 3 years ago

Consolidated the test environment variables and added a mock so we have coverage on hpa/vpa insertion even when the test OS provides them.

This may be ready to go.

jquast commented 3 years ago

Great work, thank you for all the trouble, I’ll probably be lifting some of this work into my other repos later so double thank you!

avylove commented 3 years ago

Looks like they fixed the main issue with the GitHub Actions uploader, but they haven't filtered everything out that they should, so I put in another issue and I'll leave it on the Python uploader for now.