Skip to content

Add grouping to CI logs #2772

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

Merged
merged 24 commits into from
Sep 5, 2023
Merged

Conversation

TeamSpen210
Copy link
Contributor

GitHub supports adding some annotations to stdout to allow collapsing sections of the log. This makes it easy to skip past long bits that succeeded. It'll still work fine locally, this just adds some more text to stdout and sets an environment variable.

@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Merging #2772 (746060f) into master (1c15500) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2772   +/-   ##
=======================================
  Coverage   98.94%   98.95%           
=======================================
  Files         113      115    +2     
  Lines       16919    17006   +87     
  Branches     3050     3062   +12     
=======================================
+ Hits        16741    16828   +87     
  Misses        123      123           
  Partials       55       55           
Files Changed Coverage Δ
trio/_tests/tools/test_mypy_annotate.py 100.00% <100.00%> (ø)
trio/_tools/mypy_annotate.py 100.00% <100.00%> (ø)

@jakkdl
Copy link
Member

jakkdl commented Aug 25, 2023

Is it possible to make it more apparent where it fails? Print with color? It took me quite a bit to be able to make out that it's mypy that's failing.
Though now that pre-commit is added, we should probably add https://pre-commit.ci/ to the repo? @A5rocks In my dream world that'd deprecate most/all of check.sh

@A5rocks
Copy link
Contributor

A5rocks commented Aug 25, 2023

FWIW I remember mypy passing on a pre-commit run -a but not in CI so we'd still want to run that. (specifically, in #2769 before that last commit) I am incredibly forgetful: I forgot we removed that!! Sorry about the confusion.

Also I unfortunately do not have the perms to add that :(

@TeamSpen210
Copy link
Contributor Author

Not sure about colour, but if you switch to the "summary" page of the CI run you'll see it listed there, and we can trigger github to show annotations on the actual files with messages.

@jakkdl
Copy link
Member

jakkdl commented Aug 26, 2023

Not sure about colour, but if you switch to the "summary" page of the CI run you'll see it listed there, and we can trigger github to show annotations on the actual files with messages.

Ah, took a couple clicks to get to it - but looks great! The ::error:: is great as well.

@TeamSpen210
Copy link
Contributor Author

Should write some tests for the new script first, maybe also do the same for Black & isort.

@jakkdl
Copy link
Member

jakkdl commented Aug 26, 2023

Should write some tests for the new script first, maybe also do the same for Black & isort.

I posted about pre-commit in Gitter, and in case we add it and start relying on it I'm not sure it's worth the effort to write coverage tests and polish this up too much.

Copy link
Member

@CoolCat467 CoolCat467 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 to me as long as you fix those missing return type annotation errors

@A5rocks
Copy link
Contributor

A5rocks commented Aug 30, 2023

image

Could these maybe be deduplicated?

@TeamSpen210
Copy link
Contributor Author

Not easily, each of those is a separate Mypy process fed into a separate interpeter running the script. I could make the script run Mypy itself, then gather results, but that'd move the invocations to there.

@A5rocks
Copy link
Contributor

A5rocks commented Aug 30, 2023

Maybe write stdout to a tmpfile w/ >>, cat <tmpfile> | sort | dedup? I'm not sure if that would work or not.

@CoolCat467
Copy link
Member

CoolCat467 commented Aug 30, 2023

The problem is at the moment they are separate processes, and potentially on separate VMs. It doesn't matter that much, but making them all run from one vm would make checking 3x slower as well I would think, but maybe I am not correct here.

@TeamSpen210
Copy link
Contributor Author

It's run in the same job, so they're all in the same VM, that's not the issue. dedup won't quite work because I include the platform in the message ("Mypy-Mac" etc). I could do something similar though in the script...

Copy link
Contributor

@A5rocks A5rocks left a comment

Choose a reason for hiding this comment

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

Minor comment

@jakkdl
Copy link
Member

jakkdl commented Sep 5, 2023

I'd love to get this merged and be able to use it, is there any problems remaining? I'm guessing the RTD fail is just a fluke?

@TeamSpen210
Copy link
Contributor Author

It's likely yes, earlier today GitHub broke a teeny bit, failing all the CI in rather bizarre ways. I'll do a tweak to make it run again.

@jakkdl
Copy link
Member

jakkdl commented Sep 5, 2023

Nice, feel free to merge when you're done tweaking

@TeamSpen210 TeamSpen210 enabled auto-merge (squash) September 5, 2023 10:55
@TeamSpen210 TeamSpen210 merged commit 4ea1e6d into python-trio:master Sep 5, 2023
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.

4 participants