services: stop tests before panicking

[trekhopton]

This was done because the test would panic occasionally. First step is to not let it panic.

The panic I'm referring to is #346 .

[Saxon1]

Maybe dumb question but what's the problem with it panicing ? It'll juts fail the current tetst right ?

[trekhopton]

Maybe dumb question but what's the problem with it panicing ? It'll juts fail the current tetst right ?

Yes, but that's what t.Fatalf is for imo, if code panics it usually indicates to me that something in the code is wrong and something unexpected is happening. It's better imo if test output is clear, and panics make things messy.

It's like you're trying to continue testing with invalid data because there's been an error upstream but the test was allowed to continue.

[Saxon1]

Maybe dumb question but what's the problem with it panicing ? It'll juts fail the current tetst right ?

Yes, but that's what t.Fatalf is for imo, if code panics it usually indicates to me that something in the code is wrong and something unexpected is happening. It's better imo if test output is clear, and panics make things messy.

It's like you're trying to continue testing with invalid data because there's been an error upstream but the test was allowed to continue.

hhhmm I don't think that's completely right. t.Error is when there's been a problem but you can continue the test, and t.Fatal is for when it wouldn't be meaningful to continue the test. I don't think t.Fatal should be used to avoid panics. I think panics inside functions/methods your testing is absolutely fine and you're trying to find those. Basically I'm saying your reasons for the change might be slightly off ?

I think your change is OK anyways for other reasons; in most of the places you're now using t.Fatal make sense because it wouldn't really be meaningful to continue those tests with t.Error

[trekhopton]

Maybe dumb question but what's the problem with it panicing ? It'll juts fail the current tetst right ?

Yes, but that's what t.Fatalf is for imo, if code panics it usually indicates to me that something in the code is wrong and something unexpected is happening. It's better imo if test output is clear, and panics make things messy.
It's like you're trying to continue testing with invalid data because there's been an error upstream but the test was allowed to continue.

hhhmm I don't think that's completely right. t.Error is when there's been a problem but you can continue the test, and t.Fatal is for when it wouldn't be meaningful to continue the test. I don't think t.Fatal should be used to avoid panics. I think panics inside functions/methods your testing is absolutely fine and you're trying to find those. Basically I'm saying your reasons for the change might be slightly off ?

I think your change is OK anyways for other reasons; in most of the places you're now using t.Fatal make sense because it wouldn't really be meaningful to continue those tests with t.Error

Yeah, it wouldn't be meaningful to continue the tests, I agree. That's my reason.

I get that we want to find panics if they're cause by code that we're testing, but the test code shouldn't be panicking because they didn't stop at the right time.

[Saxon1]

Maybe dumb question but what's the problem with it panicing ? It'll juts fail the current tetst right ?

Yes, but that's what t.Fatalf is for imo, if code panics it usually indicates to me that something in the code is wrong and something unexpected is happening. It's better imo if test output is clear, and panics make things messy.
It's like you're trying to continue testing with invalid data because there's been an error upstream but the test was allowed to continue.

hhhmm I don't think that's completely right. t.Error is when there's been a problem but you can continue the test, and t.Fatal is for when it wouldn't be meaningful to continue the test. I don't think t.Fatal should be used to avoid panics. I think panics inside functions/methods your testing is absolutely fine and you're trying to find those. Basically I'm saying your reasons for the change might be slightly off ?
I think your change is OK anyways for other reasons; in most of the places you're now using t.Fatal make sense because it wouldn't really be meaningful to continue those tests with t.Error

Yeah, it wouldn't be meaningful to continue the tests, I agree. That's my reason.

I get that we want to find panics if they're cause by code that we're testing, but the test code shouldn't be panicking because they didn't stop at the right time.

Yes agreed, the testing code shouldn't be panicing 😄