Is there a reason we’re excluding check connection...
# contributing-to-airbytej
Jared Rhizor (Airbyte)
12/10/2020, 5:57 PM
Is there a reason we’re excluding check connection failure tests from the standard source tests?
u
user
12/10/2020, 5:57 PM Copy code
// /**
// * Verify that when given invalid credentials, that check connection returns a failed response.
// * Assume that the {@link TestSource#getFailCheckConfig()} is invalid.
// */
// @Test
// public void testCheckConnectionInvalidCredentials() throws Exception {
// final OutputAndStatus<StandardCheckConnectionOutput> output = runCheck();
// assertTrue(output.getOutput().isPresent());
// assertEquals(Status.FAILED, output.getOutput().get().getStatus());
// }u
user
12/10/2020, 5:57 PM@charles
u
user
12/10/2020, 5:59 PMi have no problem with adding it.
u
user
12/10/2020, 5:59 PMi think the original reason i pulled it out was that there was a lot of uncertainty around how the interface with python was going to work.
u
user
12/10/2020, 6:00 PMand i wasn't sure that i wanted to ask a user to give invalid configuration.
u
user
12/10/2020, 6:00 PMlike it seemed like another confusing thing that would provide friction that didn't provide that much value since we do very little useful stuff when logging this sort of error anyway.
u
user
12/10/2020, 6:00 PMnow that the python this is more stable, i'm fine adding it back if you think it would be valuable.
u
user
12/10/2020, 6:01 PMalso fine if we leave this out until we do another iteration the standard tests and / or revamp error handling on check connection.
u
user
12/10/2020, 6:10 PMI think it would be valuable. Already ran into one case where an integration wasn’t returning a valid response due to this.
u
user
12/10/2020, 6:10 PMI’ll add it back in after the underlying issue is fixed
u
user
12/10/2020, 6:10 PMThe specific problem was an error within the logging inside the catch
u
user
12/10/2020, 6:10 PMah
u
user
12/10/2020, 6:10 PMI think generally the error handling in check connections is good
u
user
12/10/2020, 6:11 PMokay.
u
user
12/10/2020, 6:11 PMyou may need to add it optionally for now so that you don't need to edit every single source.
u
user
12/10/2020, 6:11 PMjust as a heads up.
u
user
12/10/2020, 6:17 PMIf it isn’t working for a source, it probably needs to be fixed.
u
user
12/10/2020, 6:18 PMIt means the error message won’t correctly propagate to the UI
u
user
12/10/2020, 6:18 PMsorry. i was unclear.
u
user
12/10/2020, 6:18 PMSo I’m fine adding it and then retesting all
u
user
12/10/2020, 6:18 PMAnd fixing
u
user
12/10/2020, 6:18 PMi'm assuming you are impplementing this by adding a
get_invalid_configu
user
12/10/2020, 6:18 PMwhich means you need to implement it for every source.
u
user
12/10/2020, 6:19 PMi'm just suggesting you may not want to have to do that in one go.
u
user
12/10/2020, 6:19 PMand instead figure out a way to only run that test if that is implemented.
u
user
12/10/2020, 6:19 PMoh
u
user
12/10/2020, 6:19 PMhmm
u
user
12/10/2020, 6:19 PMi'm trying to be really careful about not doing changes that require every integration to change in one commit.
u
user
12/10/2020, 6:19 PMand always have a strategy for how i can do one or a few at a time.
u
user
12/10/2020, 6:20 PMbecause we're hitting the number of integrations where doing anything for all of them in one go is just not feasible.
u
user
12/10/2020, 6:20 PMOriginally I was thinking it could just modify the correct config
u
user
12/10/2020, 6:20 PMah. you could try it 🤷 . i think it's possible to modify that config in such a way that it remains valid.
u
user
12/10/2020, 6:20 PMwe could modify every secret field
u
user
12/10/2020, 6:21 PMare there any cases where a salted hash of the secret is still valid?
u
user
12/10/2020, 6:21 PM:-p
u
user
12/10/2020, 6:21 PMi hate that we are doing more and more magic in these tests. everytime we do it bites us. @s is tearing his hair out right now on incremental on some of this stuff.
u
user
12/10/2020, 6:21 PMAlso once we add a standard test for exchange rate we’ll need to disable this test for that case specifically
u
user
12/10/2020, 6:21 PMare there any cases where a salted hash of the secret is still valid?as long as you hash it twice 😉
u
user
12/10/2020, 6:22 PMhaha
u
user
12/10/2020, 6:22 PMBut I do feel like this is a useful test because it’ll just make setup harder if the error message for bad credentials isn’t valid.
u
user
12/10/2020, 6:22 PMyeah
u
user
12/10/2020, 6:23 PMi think you have now described all of the cases i was worried about, so at least we are on the same page. 😂
u
user
12/10/2020, 6:23 PMi think i'd lean a little bit on the side of doing it explicitly. user provided invalid config, and if it is not provided we don't test it.
u
user
12/10/2020, 6:24 PMbut if you want to try the magic way, i don't think i have ground to stand on since i've committed my own magic sins in that tests suite. 😬
u
user
12/10/2020, 6:24 PMhmm
u
user
12/10/2020, 6:24 PMI feel like the Dockerization of standard tests made it really hard to edit them
u
user
12/10/2020, 6:25 PMlike there’s a separate multistep build when (I think) we don’t even have a use case that isn’t just mapping a raw file?
u
user
12/10/2020, 6:25 PMhmm
u
user
12/10/2020, 6:25 PMI guess there are exceptions
u
user
12/10/2020, 6:25 PMlike google sheets
u
user
12/10/2020, 6:25 PMwhich is a major exception
u
user
12/10/2020, 6:26 PMlol yeah.
u
user
12/10/2020, 6:26 PMi think you could argue that maybe we dockerized too soon. e.g. we've given up early velocity where we shouldn't have at the beginning.
u
user
12/10/2020, 6:26 PMbut i think it's keeping us honest.
u
user
12/10/2020, 6:27 PMit's the only way i can think of to force us to keep our tests so they can run on any connector regardless of language implementation .
u
user
12/10/2020, 6:33 PMYeah :/
u
user
12/10/2020, 6:33 PMFeels bad to either have to do something hacky or spend hours to add a trivial test case
u
user
12/10/2020, 6:34 PMGoing to do the hacky one and see if it works
3 Views