Add Support for GitHub Actions Annotations #107
Conversation
|
Hey, @pepicrft and @fortmarek! Would one of you be able to take a look at this initial implementation? I'm hoping to get some direction to bring this into a more production-ready state. Alternatively, I'd appreciate you forwarding this to someone with context! I'm seeing some issues around duplicate annotations, and I haven't settled on a general format for the output. Some initial questions around formatting:
|
There was a problem hiding this comment.
Thanks a lot @cpisciotta for taking this one on. The approach that you took looks sensible and in line with the coding practices of the repository. I left a few minor comments throughout the PR.
| func testCheckDependenciesErrors() { | ||
| } | ||
|
|
||
| func testCheckDependencies() { | ||
| } |
There was a problem hiding this comment.
Is the plan to add tests for those as well?
There was a problem hiding this comment.
I think this'll need to be a follow up PR because I don't know what the expected output is here. It wasn't defined in the pre-existing implementation (for TerminalRenderer).
That's what I'd advise for now since it's the information developers usually need to diagnose failing builds. If developers need more granularity, we can iterate on your foundation.
That's up to you to make the call. I'd advise to output the messages in a format that when presented by GitHub someone is able to tell:
Beyond that (e.g. icons), I'm not sure it adds that much value.
Similarly, I'd keep this one raw for now, and iterate from there. If you have ideas for how to leverage icons or colours to enhance the message, don't hesitate to share the options here and we can discuss what's the best one. |
|
@pepicrft Thanks for your review and the feedback on my questions! Working on a few fixes to your address your feedback and to align with your answers! |
|
@eliperkins Thanks for the feedback! Yes, that definitely makes sense to me, and I'll update to implement your suggested approach! Originally, I was trying to avoid a larger refactor for something like that, but I agree the switch statements are unideal, and it'll make for a cleaner implementation. |
cpisciotta
changed the title
cpisciotta
changed the title
| func testCheckDependenciesErrors() { | ||
| } | ||
|
|
||
| func testCheckDependencies() { | ||
| } |
There was a problem hiding this comment.
These empty tests aren't defined for the existing implementation, so totally clear on the expected output.
There was a problem hiding this comment.
There's still some missing tests from this list, but I think it's best to include those in a follow up PR, since many were already missing before I started.
|
Hi @pepicrft and @eliperkins. I think I've gotten the PR into a state ready for review and ready for production. There's quite a lot of changes, so I'm happy to walk through any areas where there's concern or questions. Working on a PR description... |
Sources/XcbeautifyLib/Parser.swift
Outdated
| public var summary: TestSummary? = nil | ||
| private(set) var summary: TestSummary? = nil |
There was a problem hiding this comment.
Since this changes a public { get set } to only a public { get }, is this a breaking change for API consumers? If so, what do we need to do to mark the next version of xcbeautify as a breaking change?
Otherwise, can we revert this change?
There was a problem hiding this comment.
This was addressed in a different PR. I don't think it requires a major release, since the only consumer of XcbeautifyLib should be the executable target. @pepicrft should confirm.
| } | ||
|
|
||
| func testCompile() { | ||
| #if os(macOS) |
There was a problem hiding this comment.
Is there a Linux branch of this that should be tested as well?
There was a problem hiding this comment.
Since this is copied from TerminalRendererTests, I'm not sure what the expected output is. I can look at this in a follow-up PR, since this is an issue for all OutputRendering types.
| XCTAssertFalse(parser.needToRecordSummary) | ||
| let formatted1 = logFormatted(input1) | ||
| #if os(macOS) | ||
| // FIXME: Failing on Linux |
There was a problem hiding this comment.
I think this should be a separate PR, since this is copied from TerminalRendererTests and also an issue there. I can look into why this is triggering an error and fix for all OutputRendering types.
|
To better separate the scope of this PR and to improve readability (for review and history), I've introduced one more intermediate PR (#124). In the new PR, I do all of the refactor work to introduce the concept of a renderer before we define a new one here. |
|
@eliperkins Are you aware of any way to filter annotations on GitHub Actions? I'm noticing a lot of noise in the summary section (namely from dependencies), so I'm trying to decide if it makes sense to add an "ignored file paths" option to xcbeautify. Alternatively, It'd be great if GitHub had this as some type of configuration option. |
cpisciotta
requested review from
a team and
danieleformichelli
and removed request for
a team
There was a problem hiding this comment.
Amazing work on this one @cpisciotta 👏🏼. I tried it locally and got the errors properly formatted:
::error file=/Users/pepicrft/Downloads/MyApp/MyApp/ContentView.swift,line=18,col=9::expected declaration
.padding()
^
::error file=/Users/pepicrft/Downloads/MyApp/MyApp/ContentView.swift,line=20,col=1::extraneous '}' at top level
}
^
::error file=/Users/pepicrft/Downloads/MyApp/MyApp/ContentView.swift,line=12,col=9::cannot find 'VStac' in scope
VStac
^~~~~
::error file=/Users/pepicrft/Downloads/MyApp/MyApp/ContentView.swift,line=20,col=1::extraneous '}' at top level
}
^
::error file=/Users/pepicrft/Downloads/MyApp/MyApp/ContentView.swift,line=18,col=9::expected declaration
|
@all-contributors add @cpisciotta for code |
|
I've put up a pull request to add @cpisciotta! 🎉 |
|
I'm not sure if this is directly related to this issue, but I'm seeing truncated results in my actions summary: Is all I get, when there should be a bunch of lines below this. |
|
@tonyarnold I think that's unrelated to my changes here. I suspect it fails for the same or a similar reason to #105. Can you give a little more context about the lines above and below? |
|
@cpisciotta yeah, it sounds like #105 — this was the full error in my build logs: |
Resolves: #38