Add coverage converting impl error to jsonreply

[arminsabouri]

Related ticket: #932

cc @benalleng as this relates to quality assurance and test coverage

[coveralls]

Pull Request Test Coverage Report for Build 16784953330

Details

• 8 of 8 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.01%) to 85.866%

---

Totals
Change from base Build 16779303392:	0.01%
Covered Lines:	7454
Relevant Lines:	8681

---

💛 - Coveralls

[benalleng]

uTACK Always nice to see some coverage expansion!

[benalleng]

Any value in maybe comparing against a dummy JsonReply struct instead of each field?

[nothingmuch]

IMO yes because there's also the extra field

perhaps my suggestion of adding

struct AlwaysPanics {};

impl fmt::Display for AlwaysPanics {
    fn fmt(&self, _f: &mut fmt::Formatter<'_>) -> fmt::Result {
        panic!("internal error should never display when converting to JsonReply");
    }
}

impl fmt::Debug for AlwaysPanics {
    fn fmt(&self, _f: &mut fmt::Formatter<'_>) -> fmt::Result {
        panic!("internal error should never debug when converting to JsonReply");
    }
}

impl Error for AlwaysPanics {
    fn source(&self) -> Option<&(dyn Error + 'static)> {
        panic!("internal error should never be examined when converting to JsonReply");
    }
}

would be better than asserting about the fields in this case?

[spacebear21]

Ben's suggestion of comparing against a struct seems like a nice middleground between low boilerplate and ensuring all fields are accounted for:

Suggested change

	assert_eq!(reply.error_code, ErrorCode::Unavailable);
	assert_eq!(reply.message, "Receiver error");
	let expected = JsonReply { error_code: ErrorCode::Unavailable, message: "Receiver error", extra: serde_json::Map::new() };
	assert_eq!(reply, expected);

Maybe to_json() should also be asserted since that's the actual value that would get transmitted to the sender?

[nothingmuch]

hmm, i think i feel a bit more strongly after sleeping on it, because it's not really JsonReply's fields we want to ensure will not have any regressions in the future, rather this is about the conversion from ReplyableError::Implementation(internal) to JsonReplyaccurate to me

[nothingmuch]

utACK, this seems sufficient, my suggestion replying to Ben is arguably more comprehensive/future proof (e.g. in case JsonReply ever gets new fields or something) but it's also more boilerplate, so i'm not married to it

[arminsabouri]

utACK, this seems sufficient, my suggestion replying to Ben is arguably more comprehensive/future proof (e.g. in case JsonReply ever gets new fields or something) but it's also more boilerplate, so i'm not married to it

Merging this now. I will leave the ticket open so we can revisit these suggestions