initial implementation of a github actions reporter #1678

devoncarew · 2022-03-18T04:12:26Z

initial implementation of a github actions reporter
address Add a Github specific reporter and enable it automatically by default based on environment variables #1415

Creating as a draft PR for now - I want to play with the output some.

pkgs/test_core/lib/src/runner/reporter/github.dart

devoncarew · 2022-03-22T21:50:21Z

OK, I think this is ready for review now. Here's the current output:

everything (tests, loading diagnostics (loadSuite), setUpAll and tearDownAll methods) gets wrapped in groups
if there's no content (errors or messages) in the synthetic (non-test) groups, they're not emitted
test successes have ✅ icons; failures have ❌ icons
synthetic groups that pass use icons that are different from tests so they're not confused with user authored tests; ⏺
skipped tests have ❎ icons; I played with many different available icons here - this seemed like the best; many others were false starts - (on github were rendered w/ a red color - indicating a failure, in a different width than the other icons, making the left edge of the output ragged, ...)
we print a success message at the end with 🎉; if a failure, we use the github ::error:: output

Some pics for the output below; others can be examined from the 'Checks' tab.

The tests for this were adopted from the ones for the other reporters. Some common ones were removed - ones that tested other functionality but that used the reporter to do that (the expanded and compact reporters still have these tests). And tests added for the github specific UI.

One thing likely to address before landing is that because of the way we collect output until a test is complete, we can re-sequence the order of stdout and error messages.

devoncarew · 2022-03-22T22:04:56Z

This reporter also doesn't have the:

Consider enabling the flag chain-stack-traces to receive more detailed exceptions.
For example, 'dart test --chain-stack-traces'.

message at the end of every run (I'm not quite sure what that is, or if there's not some other way to let users know).

natebosch · 2022-03-22T22:12:04Z

pkgs/test_core/lib/src/runner/reporter/github.dart

+
+class _GithubHelper {
+  // Char sets avilable at https://www.compart.com/en/unicode/.
+  static const String passed = '✅';


[nit] I have a slight preference for '\u2705', but I'm also curious what other folks think.

@grouma @jakemac53 @lrhn ?

'\u2705' is NOT super readable 🤷 – why the preference?

Historically they've not worked well in some editors and they aren't easy to type.
Mostly we just haven't used them in source strings that I recall, and I've only seen the \u version used.

I changed these to use the unicode escaping form.

Sorry for the late response. I think I prefer \u2705 as a constant variable with an appropriate name, e.g. _checkMark. That way it's readable and will work with all tools / editors.

One point against the '\u' form is that the Java and JavaScript style guides at google both seem to encourage it when it's "easier to read", which I think does apply here.
https://google.github.io/styleguide/javaguide.html#s2.3.3-non-ascii-characters
https://google.github.io/styleguide/jsguide.html#non-ascii-characters

C++ mentions that no user facing strings should be in source, which isn't really a practice for us, but they do still specifically allow non-ascii in source.
https://google.github.io/styleguide/cppguide.html#Non-ASCII_Characters

I'm very willing to accept that my preference is old-fashioned and choose a more modern style for our source code if that's the consensus - I mostly just wanted to double check that we weren't introducing a new pattern that would become standard without thinking it through.

@munificent - do you think this is something we should cover in Effective Dart?

Fwiw for me having the actual characters is better, I have no idea what \u2705 is and would have to look it up. I don't really see any downside to using the actual character and you have the advantage of seeing it visually.

@munificent - do you think this is something we should cover in Effective Dart?

Seems pretty nit-picky to me, but if you think it's worth it, I'm persuadable.

I think - for readability of this particular piece of code - I'm going to switch back to having the actual char in the string. It's not causing issues w/ tooling, and makes it much easier to read this bit od code.

FWIW, I'm generally comfortable with having non-ASCII characters inside Dart comments and string literals. It's entirely well-specified by the language and there's nothing intrinsically better about Unicode codepoints whose value happens to be less than 128. (I, of course, avoid homoglyphs and other nasty ones.)

pkgs/test_core/lib/src/runner/reporter/github.dart

kevmoo

Should update the changelog describing the change

Should also add something to the readme explaining how GitHub CI detection is handled – so folks aren't confused.

Looks like it's just auto-magically changed default (👍 ), so it'd be good to document that the user can override the behavior by being explicit w/ the --reporter option

devoncarew · 2022-03-23T22:47:55Z

Should update the changelog describing the change

Should also add something to the readme explaining how GitHub CI detection is handled – so folks aren't confused.

Looks like it's just auto-magically changed default (👍 ), so it'd be good to document that the user can override the behavior by being explicit w/ the --reporter option

Makes sense - I updated the changelog and added a section about test reporters to the readme.

I also rev'd the version of package:test to a pre-release version.

devoncarew · 2022-03-23T22:50:40Z

In terms of the package:test version, I rev'd this from 1.20.2 to 1.21.0-dev.

pkgs/test/README.md

devoncarew · 2022-03-23T22:58:51Z

In terms of the package:test version, I rev'd this from 1.20.2 to 1.21.0-dev.

cc @natebosch, @jakemac53 re: the version we should use here

jakemac53 · 2022-03-23T22:59:53Z

Minor version bump SGTM.

kevmoo · 2022-03-24T00:01:35Z

pkgs/test_core/lib/src/runner/reporter/github.dart

+  }
+}
+
+class _GithubHelper {


Why not just make this a abstract class w/ everything static?

Planning on having state at some point?

Or just make these top level private members

I'd like to keep these grouped with the other github markup code (startGroup(), error(), ...) as they're related. No real opinions other than that; static members works for me.

jakemac53 · 2022-03-24T14:42:13Z

pkgs/test/test/runner/github_reporter_test.dart

+            tearDownAll(() { print('two'); });
+            test('test 1', () {});
+          });''', '''
+          ::group::⏺ one (setUpAll)


Why not use ✅ for these as well? This looks to me like a warning indicator.

I would consider using this icon for skipped tests actually?

There's some (incomplete) discussion of the icons here: #1678 (comment). I did play around w/ a lot of different icons, including just editing the dom of the 'checks' page to see how things would look.

For this icon, I don't want to use the same icon as for a regular test; I think it's valuable for 6 users test ==> 6 test icons in the output. I'd like the additional test plumbing to have distinct icons from the passed tests. Additionally, the icon should be the same width as the other icons - so the LHS isn't ragged - and shouldn't use a color which users already attach some meaning to (red, yellow, ...).

Other similar unicode chars can be found here: https://www.compart.com/en/unicode/.

What about ☑? That way it is different but still indicates success (it is a test that can fail). And it isn't a color that indicates a warning. My main complaint about the current choice is the orange color indicates a warning to me.

I've been checking the rendering in-line in the test results, just so we're getting the font that github uses there. What I see for the current 'non-test' group char (⏺) is:

Which - for me - is a gray color, not an orange color. I do agree that orange (or red, or yellow) would indicate a failure or problem of some kind to the user - something we should avoid unless there's an actual issue.

Here's what the ☑ char looks like in-line in test results (note the different size and style):

Oh weird... ya its orange on a chromebook :(. I am on my mac now and I see it as a greyish blue. These were both looking at the github UI, so the color is still different based on the platform and not the terminal.

That is pretty unfortunate. @munificent was suggesting using an older check mark symbol that is just the check mark (no background) and then coloring the check mark explicitly?

The green checkmark seems to be fairly consistent so I would suggest just rendering using that. Yes you will see a different number of lines with green checkmarks than passed tests reported at the bottom, but personally that doesn't bother me (the text can still make it obvious that it is a setUpAll).

I also think we can definitely launch this with any of the suggested choices, and revist later.

that is just the check mark (no background) and then coloring the check mark explicitly?

That would be great if we could do it :) From experimentation, we can't use ansi colors in the summary line of a group here.

I also think we can definitely launch this with any of the suggested choices, and revist later.

Let's do that then; I don't think there's an idea set of icons unfortunately - I've experimented w/ many. I'll change to the green checkmark, and we can iterate as needed.

jakemac53 · 2022-03-24T14:45:31Z

pkgs/test_core/lib/src/runner/reporter/github.dart

+  }
+}
+
+class _GithubHelper {


Or just make these top level private members

pkgs/test_core/lib/src/util/io.dart

devoncarew added 2 commits March 17, 2022 21:10

initial implementation of a github actions reporter

68858c7

remove newlines from test titles

84c3797

jakemac53 reviewed Mar 18, 2022

View reviewed changes

pkgs/test_core/lib/src/runner/reporter/github.dart Outdated Show resolved Hide resolved

pkgs/test_core/lib/src/runner/reporter/github.dart Outdated Show resolved Hide resolved

some output tweaks

4554787

jakemac53 reviewed Mar 18, 2022

View reviewed changes

pkgs/test_core/lib/src/runner/reporter/github.dart Outdated Show resolved Hide resolved

devoncarew added 6 commits March 19, 2022 10:55

better visualize setUpAll, tearDownAll, and loadSuite LiveTests

e86c5b0

tweak the icons used

0dbb2b9

more teaks to icons

cb3e6d4

adjust success method and icons

cea45ef

add tests

2cbf943

fix a test in runner_test.dart

f0d7d23

kevmoo reviewed Mar 22, 2022

View reviewed changes

pkgs/test_core/lib/src/runner/reporter/github.dart Show resolved Hide resolved

devoncarew marked this pull request as ready for review March 22, 2022 21:50

natebosch reviewed Mar 22, 2022

View reviewed changes

pkgs/test_core/lib/src/runner/reporter/github.dart Outdated Show resolved Hide resolved

kevmoo requested changes Mar 23, 2022

View reviewed changes

update readme and changelog

c75e201

kevmoo reviewed Mar 23, 2022

View reviewed changes

pkgs/test/README.md Outdated Show resolved Hide resolved

kevmoo approved these changes Mar 23, 2022

View reviewed changes

Merge branch 'master' into github_actions_reporter

656e10e

devoncarew added 2 commits March 23, 2022 16:01

Update README.md

554dafa

Update github.dart

ca40a35

kevmoo reviewed Mar 24, 2022

View reviewed changes

jakemac53 reviewed Mar 24, 2022

View reviewed changes

devoncarew added 2 commits March 28, 2022 18:55

review updates

1d99df6

switch to using the checkmark icon for synthetic tests

d8d521d

jakemac53 approved these changes Mar 30, 2022

View reviewed changes

devoncarew merged commit 9c6dded into master Mar 30, 2022

jakemac53 deleted the github_actions_reporter branch March 30, 2022 15:01

parlough mentioned this pull request Apr 16, 2022

Investigate support for test package's GitHub actions reporter dart-lang/site-www#3993

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initial implementation of a github actions reporter #1678

initial implementation of a github actions reporter #1678

devoncarew commented Mar 18, 2022

devoncarew commented Mar 22, 2022 •

edited

devoncarew commented Mar 22, 2022

natebosch Mar 22, 2022

kevmoo Mar 23, 2022

natebosch Mar 23, 2022

devoncarew Mar 23, 2022

grouma Mar 24, 2022

natebosch Mar 24, 2022

jakemac53 Mar 25, 2022

munificent Mar 28, 2022

devoncarew Mar 29, 2022

munificent Mar 29, 2022

kevmoo left a comment

devoncarew commented Mar 23, 2022

devoncarew commented Mar 23, 2022

devoncarew commented Mar 23, 2022

jakemac53 commented Mar 23, 2022

kevmoo Mar 24, 2022

jakemac53 Mar 24, 2022

devoncarew Mar 29, 2022 •

edited

jakemac53 Mar 24, 2022

devoncarew Mar 29, 2022 •

edited

jakemac53 Mar 29, 2022

devoncarew Mar 29, 2022

jakemac53 Mar 29, 2022

jakemac53 Mar 29, 2022

jakemac53 Mar 29, 2022

devoncarew Mar 29, 2022

jakemac53 Mar 24, 2022

initial implementation of a github actions reporter #1678

initial implementation of a github actions reporter #1678

Conversation

devoncarew commented Mar 18, 2022

devoncarew commented Mar 22, 2022 • edited

devoncarew commented Mar 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevmoo left a comment

Choose a reason for hiding this comment

devoncarew commented Mar 23, 2022

devoncarew commented Mar 23, 2022

devoncarew commented Mar 23, 2022

jakemac53 commented Mar 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devoncarew Mar 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devoncarew Mar 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devoncarew commented Mar 22, 2022 •

edited

devoncarew Mar 29, 2022 •

edited

devoncarew Mar 29, 2022 •

edited