Add w/o grouping run in golden test #236
Conversation
sonalmahajan15
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #236 +/- ##
==========================================
- Coverage 87.49% 87.46% -0.04%
==========================================
Files 61 61
Lines 7804 7808 +4
==========================================
+ Hits 6828 6829 +1
- Misses 797 799 +2
- Partials 179 180 +1 ☔ View full report in Codecov by Sentry. |
sonalmahajan15
force-pushed
the
sonalmahajan15/add-grouping-argument-to-golden-test
branch
from
c286433
to
c51fd21
Compare
sonalmahajan15
force-pushed
the
sonalmahajan15/add-grouping-argument-to-golden-test
branch
from
8e74ba8
to
fc95ef9
Compare
| @@ -55,7 +55,7 @@ type BranchResult struct { | |||
|
|
|||
| // Run runs the golden tests on the base branch and the test branch and writes the summary and | |||
| // diff to the writer. | |||
| func Run(writer io.Writer, baseBranch, testBranch string) error { | |||
| func Run(writer io.Writer, baseBranch, testBranch, groupingFlag string) error { | |||
There was a problem hiding this comment.
nit: it would probably be better if we actually take a bool here and convert to string within this function for better type safety :)
| strategy: | ||
| matrix: | ||
| include: | ||
| - grouping: 'w/ grouping' | ||
| group_errors: 'true' | ||
| - grouping: 'w/o grouping' | ||
| group_errors: 'false' | ||
|
|
There was a problem hiding this comment.
Creating two jobs (that are running in parallel) is a good idea since golden test can easily run for several minutes.
But this would actually create two comments in the PR, which may hinder the readability for other comments.
Instead, can we keep the two jobs here (with id golden-test-w-grouping and golden-test-wo-grouping) with output, and extract the uploading step to be a separate job that depends on these two jobs, reads the output from these two jobs, and construct the message together and upload only once.
It shouldn't be too complex I think, and offers better messages for readability. What do you think?
| @@ -178,12 +178,16 @@ func ParseDiagnostics(reader io.Reader) (map[Diagnostic]bool, error) { | |||
|
|
|||
| // WriteDiff writes the summary and the diff (if the base and test are different) between the base | |||
| // and test diagnostics to the writer. If the writer is os.Stdout, it will write the diff in color. | |||
| func WriteDiff(writer io.Writer, branches [2]*BranchResult) { | |||
| func WriteDiff(writer io.Writer, branches [2]*BranchResult, groupingFlag string) { | |||
There was a problem hiding this comment.
same as above: should we just use a bool and only convert whenever we have to for better type safety?
This PR updates the golden test in CI to run under two modes: with and without grouping of similar errors.