[YUNIKORN-2123] Modify json.Unmarshal error message

[makinyemi]

What is this PR for?

Modifying the json.Unmarshal error message to better explain the failure of the specific function.

What type of PR is it?

• - Bug Fix
• - Improvement
• - Feature
• - Documentation
• - Hot Fix
• - Refactoring

Todos

• - Task

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-2123

How should this be tested?

Screenshots (if appropriate)

n/a

Questions:

n/a

[steinsgateted]

@makinyemi Thank you for the PR.
I found that there are a lot of "failed to unmarshal..."
Maybe we could make unified changes.

[makinyemi]

I found that there are a lot of "failed to unmarshal..." Maybe we could make unified changes.

@steinsgateted No problem! Ok, were you thinking unified in the sense of adding some sort of reusable function or constant string that we could format as needed to substitute for all of the occurrences of "failed to unmarshal..."?

[manirajv06]

I found that there are a lot of "failed to unmarshal..." Maybe we could make unified changes.

@steinsgateted No problem! Ok, were you thinking unified in the sense of adding some sort of reusable function or constant string that we could format as needed to substitute for all of the occurrences of "failed to unmarshal..."?

Introduce a const something like this.
STRING1 = "failed to unmarshal DAONAME response from response body"
replace DAONAME with actual name.

Also you can introduce constants for

"Incorrect Status code"
"JSON error message is incorrect"

[codecov]

Codecov Report

Merging #698 (b37f8b4) into master (30e69f9) will increase coverage by 0.05%.
Report is 5 commits behind head on master.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #698      +/-   ##
==========================================
+ Coverage   77.63%   77.69%   +0.05%     
==========================================
  Files          80       80              
  Lines       13271    13320      +49     
==========================================
+ Hits        10303    10349      +46     
- Misses       2648     2650       +2     
- Partials      320      321       +1     

see 9 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

[craigcondit]

-1. Every one of of the new messages is incorrect. It's the YAPIError structure that we're attempting to parse (Contains StatusCode, Message, Description). All of these functions are used to test error handling, and we assert in each of them that the error object is parsed correctly. Just change every message to "failed to unmarshal error response from response body".

[craigcondit]

Now you've got additional changes that are not error handling. You need to pay attention to the context for each one.

[makinyemi]

Now you've got additional changes that are not error handling. You need to pay attention to the context for each one.

Thanks for the feedback. Will reset and go back through

[craigcondit]

+1, pending precommit checks.

[steinsgateted]

@makinyemi @manirajv06 @craigcondit
Thank you for your discussion.
nit: I think other "failed to unmarshal" information can also be changed.

[makinyemi]

nit: I think other "failed to unmarshal" information can also be changed.

Similar to my last commit? Make a constant for specific test functions?

[steinsgateted]

Just change every message to "failed to unmarshal error response from response body".

Similar to my last commit? Make a constant for specific test functions?

@makinyemi @craigcondit
Thank you.
I think it's like @craigcondit said.
Replace all messages with unmarshalError.
No need to retain the original DAONAME part.

[steinsgateted]

UnmarshalError -> unmarshalError

[makinyemi]

Replace all messages with unmarshalError. No need to retain the original DAONAME part.

@steinsgateted I did something similar to that in a previous commit that i reverted where @craigcondit mentioned in the above comment

Now you've got additional changes that are not error handling. You need to pay attention to the context for each one.

If i understand correctly, as @craigcondit explained it certain test were testing for specific failure and the messages reflected the error they were handling.

If i am mistaken i can definitely make the corrections

[makinyemi]

@steinsgateted @craigcondit Actually, I remember now. I added DAOInfo to every message, which was wrong.
So if i understand correctly. We are good to remove all DAOInfo?

[steinsgateted]

@makinyemi Thank you
I think we can use two variables to deal with different situations.
Create a new variable:
const unmarshalDAOError = "Failed to unmarshal dao response from response body"
In test error handling situation:
assert.NilError(t, err, unmarshalError)
in dao:
assert.NilError(t, err, "%s (ValidateConfResponse)", unmarshalDAOError)

Also you can introduce constants for

"Incorrect Status code"
"JSON error message is incorrect"

we could add two new consts:

const statusCodeError = "Incorrect Status code"
const jsonMessageError = "JSON error message is incorrect"

[steinsgateted]

@makinyemi @manirajv06 @craigcondit Thank you.
I would like some clarifications.
I think there are three expressions of unmarshal dao messages.
Which expression method do you think makes more sense?

• Using the original method is like:

assert.NilError(t, err, "failed to unmarshal ValidateConfResponse from response body")

• assert.NilError(t, err, "%s (ValidateConfResponse)", unmarshalDAOError)

• assert.NilError(t, err, unmarshalDAOError)

[craigcondit]

@makinyemi @manirajv06 @craigcondit Thank you. I would like some clarifications. I think there are three expressions of unmarshal dao messages. Which expression method do you think makes more sense?

• Using the original method is like:

assert.NilError(t, err, "failed to unmarshal ValidateConfResponse from response body")

• assert.NilError(t, err, "%s (ValidateConfResponse)", unmarshalDAOError)
• assert.NilError(t, err, unmarshalDAOError)

What I think is that far too much time and effort has been spent on this. We're talking about the wording of a diagnostic message echoed by test cases when they fail (which they basically never do). Pick something reasonable and go with it.

[steinsgateted]

@makinyemi @craigcondit Thank you. +1

[pbacsko]

+1