Fix- Print error messages in pre builder #829

brunasilvazup · 2021-01-15T20:38:33Z

Description

This exposes the errors associated with the pre-build, improving the traceability and visibility of the error content.

Some extra improvements were the addition of new mocks and asserts in the tests.

In this commit an intermittent test was corrected. -> 972f69b
For the names of the mock files I followed the name of the corresponding package

How to verify it

Check the code or clone this pr, make the build and tests errors on build.

Changelog

Print error message in pre-builder

Signed-off-by: Bruna Tavares <bruna.silva@zup.com.br>

codecov-io · 2021-01-15T20:44:20Z

Codecov Report

Merging #829 (2d9cf86) into master (37ab4d1) will increase coverage by 0.39%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master     #829      +/-   ##
==========================================
+ Coverage   84.34%   84.73%   +0.39%     
==========================================
  Files         111      111              
  Lines        3857     3859       +2     
==========================================
+ Hits         3253     3270      +17     
+ Misses        432      419      -13     
+ Partials      172      170       -2

Impacted Files	Coverage Δ
pkg/formula/runner.go	`0.00% <ø> (ø)`
pkg/formula/runner/executor.go	`88.23% <0.00%> (+0.73%)`	⬆️
pkg/formula/runner/pre_run_builder.go	`100.00% <100.00%> (+35.71%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37ab4d1...2d9cf86. Read the comment docs.

henriquemoraeszup · 2021-01-19T19:46:08Z

internal/mocks/mocks.go

+type BuilderMock struct {
+	mock.Mock
+}
+
+func (b *BuilderMock) Build(info formula.BuildInfo) error {
+	args := b.Called(info)
+	return args.Error(0)
+}
+func (b *BuilderMock) HasBuilt() bool {
+	args := b.Called()
+	return args.Bool(0)
+}


Can you move this to a dedicated file in mocks with the package name? The mocks.go file is getting large already

henriquemoraeszup · 2021-01-19T19:50:08Z

pkg/formula/runner/pre_run_builder_test.go

-func (bm builderMock) HasBuilt() bool {
-	return *bm.hasBuilt
-}
-
 type workspaceListHasherMock struct {


should we strive to eliminate this guy?

kaduartur · 2021-01-19T21:37:44Z

pkg/formula/runner/pre_run_builder.go

@@ -47,7 +47,8 @@ func NewPreRunBuilder(
 func (b PreRunBuilderManager) Build(relativePath string) {


I think you could return an error in this method, it will make your tests easier.

kaduartur · 2021-01-19T21:39:46Z

pkg/formula/runner/pre_run_builder.go

@@ -57,7 +58,8 @@ func (b PreRunBuilderManager) Build(relativePath string) {
 	}

 	if err = b.buildOnWorkspace(*workspace, relativePath); err != nil {
-		fmt.Println(prompt.Red(messageBuildError))
+		msg := fmt.Sprintf(messageBuildError, err.Error())
+		fmt.Println(prompt.Red(msg))


Here it makes sense for you to return the error, so you prevent its repetition for the user.

I will be change this method to return error.

Signed-off-by: Bruna Tavares <bruna.silva@zup.com.br>

…x/print-error-messages-in-pre-builder Signed-off-by: Bruna Tavares <bruna.silva@zup.com.br>

intermittent Signed-off-by: Bruna Tavares <bruna.silva@zup.com.br>

Signed-off-by: Bruna Tavares <bruna.silva@zup.com.br>

…x/print-error-messages-in-pre-builder Signed-off-by: Bruna Tavares <bruna.silva@zup.com.br>

henriquemoraeszup

Nice, just a single comment

henriquemoraeszup · 2021-01-26T18:34:59Z

pkg/cmd/add_repo.go

@@ -287,6 +288,7 @@ func (ar *addRepoCmd) resolveFlags(cmd *cobra.Command) (formula.Repo, error) {
 		}
 	}
 	if !providerValid {
+		sort.Strings(providers)


can you move the sort to inside the ar.repoProviders.List() method? It should not be the responsibility of this command to know that. Also, this way we only need to edit a single point in code, other packages invoking this method could stumble upon the same problem

Yes, I can change, you're right.

henriquemoraeszup · 2021-01-26T18:40:48Z

Can you add this PR to the pre run test of #788?

Signed-off-by: Bruna Tavares <bruna.silva@zup.com.br>

brunasilvazup · 2021-01-26T20:49:21Z

/merge qa

ritchie-bot · 2021-01-26T20:50:05Z

🔥 Merge Conflict

…x/print-error-messages-in-pre-builder Signed-off-by: Bruna Tavares <bruna.silva@zup.com.br>

Signed-off-by: Bruna Tavares <bruna.silva@zup.com.br>

ritchie-bot · 2021-02-05T13:41:23Z

🔥 Merge Conflict

kaduartur

LGTM 🚀

brunasilvazup added 2 commits January 15, 2021 17:32

Fix error message presentation

d2df0ee

Signed-off-by: Bruna Tavares <bruna.silva@zup.com.br>

Mocks tests

15e6ceb

Signed-off-by: Bruna Tavares <bruna.silva@zup.com.br>

brunasilvazup linked an issue Jan 15, 2021 that may be closed by this pull request

Expose specific error message during build #801

Closed

brunasilvazup self-assigned this Jan 18, 2021

brunasilvazup added the 🪲 bug Report a bug encountered while operating Ritchie label Jan 18, 2021

brunasilvazup changed the title ~~Fix/print error messages in pre builder~~ Fix- Print error messages in pre builder Jan 18, 2021

brunasilvazup marked this pull request as ready for review January 18, 2021 19:46

brunasilvazup requested review from henriquemoraeszup, kaduartur, lucasdittrichzup and rodrigomedeirosf as code owners January 18, 2021 19:46

brunasilvazup added the ✔️ ready-for-review ready for review label Jan 18, 2021

henriquemoraeszup reviewed Jan 19, 2021

View reviewed changes

kaduartur suggested changes Jan 19, 2021

View reviewed changes

lucasdittrichzup added waiting reply Waiting for an answer to a comment and removed ✔️ ready-for-review ready for review labels Jan 21, 2021

brunasilvazup added 5 commits January 21, 2021 17:31

Extracts mock functions to specific file

4a973d7

Signed-off-by: Bruna Tavares <bruna.silva@zup.com.br>

Changed the prerunbuilders build method signature to return errors

fc1bb61

Signed-off-by: Bruna Tavares <bruna.silva@zup.com.br>

Changed mock tests to testify

2564baa

Signed-off-by: Bruna Tavares <bruna.silva@zup.com.br>

Added more tests

17375e8

Signed-off-by: Bruna Tavares <bruna.silva@zup.com.br>

Fix lint

ca63571

Signed-off-by: Bruna Tavares <bruna.silva@zup.com.br>

brunasilvazup added 🚧 WIP Work in Progress and removed waiting reply Waiting for an answer to a comment labels Jan 22, 2021

brunasilvazup added 6 commits January 22, 2021 17:56

Added more tests

2d9cf86

Signed-off-by: Bruna Tavares <bruna.silva@zup.com.br>

Merge

de3456a

Signed-off-by: Bruna Tavares <bruna.silva@zup.com.br>

Merge branch 'master' of github.com:brunasilvazup/ritchie-cli into fi…

e5ef80a

…x/print-error-messages-in-pre-builder Signed-off-by: Bruna Tavares <bruna.silva@zup.com.br>

Fix test

972f69b

intermittent Signed-off-by: Bruna Tavares <bruna.silva@zup.com.br>

Update mock files

4ad033f

Signed-off-by: Bruna Tavares <bruna.silva@zup.com.br>

Added more tests

aedbcc5

Signed-off-by: Bruna Tavares <bruna.silva@zup.com.br>

brunasilvazup added ✔️ ready-for-review ready for review and removed 🚧 WIP Work in Progress labels Jan 26, 2021

brunasilvazup added 2 commits January 26, 2021 14:09

Fix small details

94f6f8c

Signed-off-by: Bruna Tavares <bruna.silva@zup.com.br>

Merge branch 'master' of github.com:brunasilvazup/ritchie-cli into fi…

51423db

…x/print-error-messages-in-pre-builder Signed-off-by: Bruna Tavares <bruna.silva@zup.com.br>

brunasilvazup requested review from henriquemoraeszup and kaduartur January 26, 2021 17:11

lucasdittrichzup previously approved these changes Jan 26, 2021

View reviewed changes

henriquemoraeszup previously approved these changes Jan 26, 2021

View reviewed changes

brunasilvazup mentioned this pull request Jan 26, 2021

Tests with Testify in Pkg/Formula/Runner #788

Closed

5 tasks

Extracts sort from the list of providers for method list

2cc252b

Signed-off-by: Bruna Tavares <bruna.silva@zup.com.br>

brunasilvazup dismissed stale reviews from henriquemoraeszup and lucasdittrichzup via 2cc252b January 26, 2021 18:53

brunasilvazup requested review from lucasdittrichzup and henriquemoraeszup January 26, 2021 18:56

lucasdittrichzup previously approved these changes Jan 26, 2021

View reviewed changes

Merge branch 'master' of github.com:brunasilvazup/ritchie-cli into fi…

a734cc3

…x/print-error-messages-in-pre-builder Signed-off-by: Bruna Tavares <bruna.silva@zup.com.br>

brunasilvazup added 🚧 WIP Work in Progress and removed ✔️ ready-for-review ready for review labels Jan 27, 2021

Remove extra sort

7ca2eb8

Signed-off-by: Bruna Tavares <bruna.silva@zup.com.br>

brunasilvazup dismissed lucasdittrichzup’s stale review via 7ca2eb8 January 28, 2021 13:16

brunasilvazup added 🚀 QA and removed 🚧 WIP Work in Progress labels Feb 5, 2021

ZupIT deleted a comment from ritchie-bot bot Feb 5, 2021

kaduartur approved these changes Feb 8, 2021

View reviewed changes

brunasilvazup added the ✔️ ready-for-review ready for review label Feb 8, 2021

brunasilvazup merged commit ceca367 into ZupIT:master Feb 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix- Print error messages in pre builder #829

Fix- Print error messages in pre builder #829

brunasilvazup commented Jan 15, 2021 •

edited

Loading

codecov-io commented Jan 15, 2021 •

edited

Loading

henriquemoraeszup Jan 19, 2021

henriquemoraeszup Jan 19, 2021

kaduartur Jan 19, 2021

kaduartur Jan 19, 2021

brunasilvazup Jan 22, 2021

henriquemoraeszup left a comment

henriquemoraeszup Jan 26, 2021

brunasilvazup Jan 26, 2021

henriquemoraeszup commented Jan 26, 2021 •

edited

Loading

brunasilvazup commented Jan 26, 2021

ritchie-bot bot commented Jan 26, 2021

ritchie-bot bot commented Feb 5, 2021

kaduartur left a comment

		@@ -47,7 +47,8 @@ func NewPreRunBuilder(
		func (b PreRunBuilderManager) Build(relativePath string) {

Fix- Print error messages in pre builder #829

Fix- Print error messages in pre builder #829

Conversation

brunasilvazup commented Jan 15, 2021 • edited Loading

Description

How to verify it

Changelog

codecov-io commented Jan 15, 2021 • edited Loading

Codecov Report

henriquemoraeszup Jan 19, 2021

Choose a reason for hiding this comment

henriquemoraeszup Jan 19, 2021

Choose a reason for hiding this comment

kaduartur Jan 19, 2021

Choose a reason for hiding this comment

kaduartur Jan 19, 2021

Choose a reason for hiding this comment

brunasilvazup Jan 22, 2021

Choose a reason for hiding this comment

henriquemoraeszup left a comment

Choose a reason for hiding this comment

henriquemoraeszup Jan 26, 2021

Choose a reason for hiding this comment

brunasilvazup Jan 26, 2021

Choose a reason for hiding this comment

henriquemoraeszup commented Jan 26, 2021 • edited Loading

brunasilvazup commented Jan 26, 2021

ritchie-bot bot commented Jan 26, 2021

ritchie-bot bot commented Feb 5, 2021

kaduartur left a comment

Choose a reason for hiding this comment

brunasilvazup commented Jan 15, 2021 •

edited

Loading

codecov-io commented Jan 15, 2021 •

edited

Loading

henriquemoraeszup commented Jan 26, 2021 •

edited

Loading