Skip to content

Commit d0a9999

Browse files
daveshanleydaveshanley
committed
Address issue #220
Clarity over what is going on and replaced left / right with original and modified
1 parent 9b5083d commit d0a9999

14 files changed

Lines changed: 208 additions & 50 deletions

‎cmd/engine.go‎

Lines changed: 71 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -45,11 +45,11 @@ func runChangerator(commit *model.Commit, breakingConfig *whatChangedModel.Break
4545

4646
rightModel, err := commit.Document.BuildV3Model()
4747
if err != nil {
48-
return nil, fmt.Errorf("building right model: %w", err)
48+
return nil, modelBuildError("modified", commitSourceLabel(commit, true), err)
4949
}
5050
leftModel, err := commit.OldDocument.BuildV3Model()
5151
if err != nil {
52-
return nil, fmt.Errorf("building left model: %w", err)
52+
return nil, modelBuildError("original", commitSourceLabel(commit, false), err)
5353
}
5454

5555
rightDrDoc := drModel.NewDrDocumentAndGraph(rightModel)
@@ -180,6 +180,73 @@ func rewriteDocumentLocation(raw string, rewriters []model.DocumentPathRewriter)
180180
return raw
181181
}
182182

183-
func emitCommitWarning(commitHash string, err error) {
184-
fmt.Fprintf(os.Stderr, "warning: commit %s: %s\n", commitHash, err)
183+
func modelBuildError(side, label string, err error) error {
184+
if label == "" {
185+
return fmt.Errorf("building %s model: %w", side, err)
186+
}
187+
return fmt.Errorf("building %s model '%s': %w", side, label, err)
188+
}
189+
190+
func commitSourceLabel(commit *model.Commit, modified bool) string {
191+
if commit == nil {
192+
return ""
193+
}
194+
if modified {
195+
if commit.ModifiedSource != "" {
196+
return commit.ModifiedSource
197+
}
198+
if commit.FilePath != "" {
199+
return commit.FilePath
200+
}
201+
return ""
202+
}
203+
if commit.OriginalSource != "" {
204+
return commit.OriginalSource
205+
}
206+
if commit.FilePath != "" {
207+
return commit.FilePath
208+
}
209+
return ""
210+
}
211+
212+
func commitContextLabel(commit *model.Commit) string {
213+
if commit == nil {
214+
return ""
215+
}
216+
if commit.Synthetic {
217+
switch {
218+
case commit.OriginalSource != "" && commit.ModifiedSource != "":
219+
return fmt.Sprintf("comparison '%s' -> '%s'", commit.OriginalSource, commit.ModifiedSource)
220+
case commit.ModifiedSource != "":
221+
return fmt.Sprintf("comparison '%s'", commit.ModifiedSource)
222+
case commit.OriginalSource != "":
223+
return fmt.Sprintf("comparison '%s'", commit.OriginalSource)
224+
}
225+
}
226+
if commit.Hash != "" && commit.FilePath != "" {
227+
return fmt.Sprintf("commit %s (%s)", commit.Hash, commit.FilePath)
228+
}
229+
if commit.Hash != "" {
230+
return fmt.Sprintf("commit %s", commit.Hash)
231+
}
232+
if commit.FilePath != "" {
233+
return fmt.Sprintf("file '%s'", commit.FilePath)
234+
}
235+
return ""
236+
}
237+
238+
func wrapCommitError(commit *model.Commit, err error) error {
239+
if err == nil {
240+
return nil
241+
}
242+
if label := commitContextLabel(commit); label != "" {
243+
return fmt.Errorf("%s: %w", label, err)
244+
}
245+
return err
246+
}
247+
248+
func emitCommitWarning(commit *model.Commit, err error) {
249+
if wrapped := wrapCommitError(commit, err); wrapped != nil {
250+
fmt.Fprintf(os.Stderr, "warning: %s\n", wrapped)
251+
}
185252
}

‎cmd/html_report.go‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,8 @@ func buildHTMLReportItems(commits []*model.Commit, breakingConfig *whatChangedMo
3737
for i, commit := range commits {
3838
result, err := runChangerator(commit, breakingConfig)
3939
if err != nil {
40-
emitCommitWarning(commit.Hash, err)
41-
buildErrors = append(buildErrors, err)
40+
emitCommitWarning(commit, err)
41+
buildErrors = append(buildErrors, wrapCommitError(commit, err))
4242
continue
4343
}
4444
if result == nil {
@@ -56,8 +56,9 @@ func buildHTMLReportItems(commits []*model.Commit, breakingConfig *whatChangedMo
5656
result.Release()
5757

5858
if err != nil {
59-
emitCommitWarning(commit.Hash, fmt.Errorf("building report item: %w", err))
60-
buildErrors = append(buildErrors, err)
59+
wrappedErr := fmt.Errorf("building report item: %w", err)
60+
emitCommitWarning(commit, wrappedErr)
61+
buildErrors = append(buildErrors, wrapCommitError(commit, wrappedErr))
6162
continue
6263
}
6364

‎cmd/html_report_test.go‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,25 @@ func TestBuildHTMLReportItems_AllCommitsFail(t *testing.T) {
130130
assert.Contains(t, err.Error(), "all 1 commits failed to build report items")
131131
}
132132

133+
func TestBuildHTMLReportItems_LeftRightModelBuildErrorNamesFiles(t *testing.T) {
134+
leftPath, rightPath := createBrokenReferenceSpecPair(t)
135+
136+
commits, err := loadLeftRightCommits(leftPath, rightPath, summaryOpts{noColor: true})
137+
require.NoError(t, err)
138+
139+
var buildErr error
140+
stderr := captureStderr(t, func() {
141+
_, buildErr = buildHTMLReportItems(commits, nil)
142+
})
143+
144+
require.Error(t, buildErr)
145+
assert.Contains(t, buildErr.Error(), "all 1 commits failed to build report items")
146+
assert.Contains(t, buildErr.Error(), rightPath)
147+
assert.Contains(t, buildErr.Error(), "building modified model")
148+
assert.Contains(t, stderr, leftPath)
149+
assert.Contains(t, stderr, rightPath)
150+
}
151+
133152
func TestBuildHTMLReportItems_PartialFailureReturnsPartialResults(t *testing.T) {
134153
commits, err := loadLeftRightCommits(
135154
"../sample-specs/petstorev3-original.json",
@@ -147,7 +166,7 @@ func TestBuildHTMLReportItems_PartialFailureReturnsPartialResults(t *testing.T)
147166
})
148167
require.NoError(t, err)
149168
assert.NotEmpty(t, items, "should return successfully built items despite partial failure")
150-
assert.Contains(t, stderr, "warning: commit abc123: building right model")
169+
assert.Contains(t, stderr, "warning: commit abc123: building modified model")
151170
assert.Contains(t, stderr, "warning: 1 commits failed to build report items")
152171
}
153172

‎cmd/left_right_sources.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -279,6 +279,8 @@ func buildLeftRightCommit(left, right comparisonSource) (*model.Commit, error) {
279279
Data: right.RootBytes,
280280
OldDocument: leftDoc,
281281
Document: rightDoc,
282+
OriginalSource: left.Display,
283+
ModifiedSource: right.Display,
282284
Synthetic: true,
283285
DocumentRewriters: rewriters,
284286
}, nil

‎cmd/markdown_report.go‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
package cmd
55

66
import (
7+
"errors"
78
"fmt"
89
"os"
910
"strings"
@@ -81,15 +82,15 @@ func isSyntheticLeftRightCommit(commit *model.Commit) bool {
8182
func generateMarkdownReport(commits []*model.Commit, breakingConfig *whatChangedModel.BreakingRulesConfig, includeDiff bool) ([]byte, error) {
8283
var sb strings.Builder
8384
successCount := 0
84-
errorCount := 0
85+
var renderErrors []error
8586
headerWritten := false
8687
includeCommitMetadata := true
8788

8889
for i, commit := range commits {
8990
markdown, err := renderCommitMarkdown(commit, breakingConfig)
9091
if err != nil {
91-
emitCommitWarning(commit.Hash, err)
92-
errorCount++
92+
emitCommitWarning(commit, err)
93+
renderErrors = append(renderErrors, wrapCommitError(commit, err))
9394
continue
9495
}
9596
if markdown == "" {
@@ -138,13 +139,13 @@ func generateMarkdownReport(commits []*model.Commit, breakingConfig *whatChanged
138139
sb.WriteString("---\n\n")
139140
}
140141

141-
if successCount == 0 && errorCount > 0 {
142-
return nil, fmt.Errorf("all %d commits failed to render", errorCount)
142+
if successCount == 0 && len(renderErrors) > 0 {
143+
return nil, fmt.Errorf("all %d commits failed to render: %w", len(renderErrors), errors.Join(renderErrors...))
143144
}
144-
if errorCount > 0 {
145-
fmt.Fprintf(os.Stderr, "warning: %d commits failed to render\n", errorCount)
145+
if len(renderErrors) > 0 {
146+
fmt.Fprintf(os.Stderr, "warning: %d commits failed to render\n", len(renderErrors))
146147
}
147-
if successCount == 0 && errorCount == 0 {
148+
if successCount == 0 && len(renderErrors) == 0 {
148149
return nil, nil
149150
}
150151
return []byte(sb.String()), nil

‎cmd/markdown_report_test.go‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,26 @@ paths: {}`
5656
assert.Contains(t, err.Error(), "all 1 commits failed to render")
5757
}
5858

59+
func TestMarkdownReport_LeftRightModelBuildErrorNamesFiles(t *testing.T) {
60+
leftPath, rightPath := createBrokenReferenceSpecPair(t)
61+
62+
commits, err := loadLeftRightCommits(leftPath, rightPath, summaryOpts{noColor: true})
63+
require.NoError(t, err)
64+
65+
var report []byte
66+
stderr := captureStderr(t, func() {
67+
report, err = generateMarkdownReport(commits, nil, false)
68+
})
69+
70+
require.Error(t, err)
71+
assert.Nil(t, report)
72+
assert.Contains(t, err.Error(), "all 1 commits failed to render")
73+
assert.Contains(t, err.Error(), rightPath)
74+
assert.Contains(t, err.Error(), "building modified model")
75+
assert.Contains(t, stderr, leftPath)
76+
assert.Contains(t, stderr, rightPath)
77+
}
78+
5979
func TestMarkdownReport_HeadingStripping(t *testing.T) {
6080
opts := summaryOpts{noColor: true}
6181
commits, err := loadLeftRightCommits(
@@ -197,7 +217,7 @@ paths: {}`
197217
})
198218
assert.NoError(t, reportErr)
199219
assert.NotNil(t, report, "should return partial report with successfully rendered commits")
200-
assert.Contains(t, stderr, "warning: commit bad123: building right model")
220+
assert.Contains(t, stderr, "warning: commit bad123: building modified model")
201221
assert.Contains(t, stderr, "warning: 1 commits failed to render")
202222
}
203223

‎cmd/report.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ import (
2222
func changerateCommit(commit *model.Commit, breakingConfig *whatChangedModel.BreakingRulesConfig) (*changeratorResult, error) {
2323
result, err := runChangerator(commit, breakingConfig)
2424
if err != nil {
25-
return nil, fmt.Errorf("commit %s: %w", commit.Hash, err)
25+
return nil, wrapCommitError(commit, err)
2626
}
2727
if result == nil {
2828
return nil, nil

‎cmd/report_test.go‎

Lines changed: 4 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -20,34 +20,15 @@ import (
2020
)
2121

2222
func TestRunLeftRightReport_PropagatesChangeratorErrors(t *testing.T) {
23-
dir := t.TempDir()
24-
25-
left := `swagger: "2.0"
26-
info:
27-
title: test
28-
version: "1.0"
29-
paths: {}`
30-
right := `swagger: "2.0"
31-
info:
32-
title: test updated
33-
version: "1.1"
34-
paths:
35-
/pets:
36-
get:
37-
responses:
38-
"200":
39-
description: ok`
40-
41-
leftPath := filepath.Join(dir, "left.yaml")
42-
rightPath := filepath.Join(dir, "right.yaml")
43-
require.NoError(t, os.WriteFile(leftPath, []byte(left), 0644))
44-
require.NoError(t, os.WriteFile(rightPath, []byte(right), 0644))
23+
leftPath, rightPath := createBrokenReferenceSpecPair(t)
4524

4625
report, err := runLeftRightReport(leftPath, rightPath, summaryOpts{}, nil)
4726

4827
require.Error(t, err)
4928
assert.Nil(t, report)
50-
assert.Contains(t, err.Error(), "building right model")
29+
assert.Contains(t, err.Error(), leftPath)
30+
assert.Contains(t, err.Error(), rightPath)
31+
assert.Contains(t, err.Error(), "building modified model")
5132
}
5233

5334
func TestRunGithubHistoryReport_PropagatesChangeratorErrors(t *testing.T) {

‎cmd/summary.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -301,8 +301,8 @@ func renderSummaryWithTheme(
301301

302302
result, err := runChangerator(commit, breakingConfig)
303303
if err != nil {
304-
emitCommitWarning(commit.Hash, err)
305-
renderErrors = append(renderErrors, fmt.Errorf("commit %s: %w", commit.Hash, err))
304+
emitCommitWarning(commit, err)
305+
renderErrors = append(renderErrors, wrapCommitError(commit, err))
306306
continue
307307
}
308308
if result == nil {

‎cmd/summary_test.go‎

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,7 @@ func TestRenderSummary_ReturnsErrorWhenAllCommitsFailToRender(t *testing.T) {
183183

184184
require.Error(t, err)
185185
assert.Contains(t, err.Error(), "all 1 commits failed to render")
186-
assert.Contains(t, stderr, "warning: commit abc123: building right model")
186+
assert.Contains(t, stderr, "warning: commit abc123: building modified model")
187187
assert.Empty(t, output)
188188
assert.False(t, hasBreaking)
189189
assert.False(t, hasChanges)
@@ -221,7 +221,7 @@ paths: {}
221221
require.NoError(t, err)
222222
assert.Contains(t, output, "Date:")
223223
assert.NotContains(t, output, "Error:")
224-
assert.Contains(t, stderr, "warning: commit abc123: building right model")
224+
assert.Contains(t, stderr, "warning: commit abc123: building modified model")
225225
assert.Contains(t, stderr, "warning: 1 commits failed to render")
226226
assert.False(t, hasBreaking)
227227
assert.True(t, hasChanges)
@@ -243,6 +243,33 @@ func TestLoadLeftRightCommits_IdenticalSpecsPreserveComparableRevision(t *testin
243243
assert.Nil(t, commits[0].Changes)
244244
}
245245

246+
func TestRenderSummary_LeftRightModelBuildErrorNamesModifiedFile(t *testing.T) {
247+
leftPath, rightPath := createBrokenReferenceSpecPair(t)
248+
249+
commits, err := loadLeftRightCommits(leftPath, rightPath, summaryOpts{noColor: true})
250+
require.NoError(t, err)
251+
require.Len(t, commits, 1)
252+
253+
var renderErr error
254+
stderr := captureStderr(t, func() {
255+
_, _, _, renderErr = renderSummary(
256+
commits,
257+
nil,
258+
false,
259+
true,
260+
false,
261+
summaryStyles{},
262+
)
263+
})
264+
265+
require.Error(t, renderErr)
266+
assert.Contains(t, renderErr.Error(), "all 1 commits failed to render")
267+
assert.Contains(t, renderErr.Error(), rightPath)
268+
assert.Contains(t, renderErr.Error(), "building modified model")
269+
assert.Contains(t, stderr, leftPath)
270+
assert.Contains(t, stderr, rightPath)
271+
}
272+
246273
func TestRenderSummary_DirectComparisonNoChangesUsesGenericMessage(t *testing.T) {
247274
commits, err := loadLeftRightCommits(
248275
"../sample-specs/petstorev3.json",

0 commit comments

Comments
 (0)