https://linen.dev logo
j

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 PM
i have no problem with adding it.
u

user

12/10/2020, 5:59 PM
i 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 PM
and i wasn't sure that i wanted to ask a user to give invalid configuration.
u

user

12/10/2020, 6:00 PM
like 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 PM
now 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 PM
also 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 PM
I 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 PM
I’ll add it back in after the underlying issue is fixed
u

user

12/10/2020, 6:10 PM
The specific problem was an error within the logging inside the catch
u

user

12/10/2020, 6:10 PM
ah
u

user

12/10/2020, 6:10 PM
I think generally the error handling in check connections is good
u

user

12/10/2020, 6:11 PM
okay.
u

user

12/10/2020, 6:11 PM
you 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 PM
just as a heads up.
u

user

12/10/2020, 6:17 PM
If it isn’t working for a source, it probably needs to be fixed.
u

user

12/10/2020, 6:18 PM
It means the error message won’t correctly propagate to the UI
u

user

12/10/2020, 6:18 PM
sorry. i was unclear.
u

user

12/10/2020, 6:18 PM
So I’m fine adding it and then retesting all
u

user

12/10/2020, 6:18 PM
And fixing
u

user

12/10/2020, 6:18 PM
i'm assuming you are impplementing this by adding a
get_invalid_config
method to the python iface.
u

user

12/10/2020, 6:18 PM
which means you need to implement it for every source.
u

user

12/10/2020, 6:19 PM
i'm just suggesting you may not want to have to do that in one go.
u

user

12/10/2020, 6:19 PM
and instead figure out a way to only run that test if that is implemented.
u

user

12/10/2020, 6:19 PM
oh
u

user

12/10/2020, 6:19 PM
hmm
u

user

12/10/2020, 6:19 PM
i'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 PM
and always have a strategy for how i can do one or a few at a time.
u

user

12/10/2020, 6:20 PM
because 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 PM
Originally I was thinking it could just modify the correct config
u

user

12/10/2020, 6:20 PM
ah. 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 PM
we could modify every secret field
u

user

12/10/2020, 6:21 PM
are 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 PM
i 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 PM
Also 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 PM
are 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 PM
haha
u

user

12/10/2020, 6:22 PM
But 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 PM
yeah
u

user

12/10/2020, 6:23 PM
i 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 PM
i 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 PM
but 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 PM
hmm
u

user

12/10/2020, 6:24 PM
I feel like the Dockerization of standard tests made it really hard to edit them
u

user

12/10/2020, 6:25 PM
like 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 PM
hmm
u

user

12/10/2020, 6:25 PM
I guess there are exceptions
u

user

12/10/2020, 6:25 PM
like google sheets
u

user

12/10/2020, 6:25 PM
which is a major exception
u

user

12/10/2020, 6:26 PM
lol yeah.
u

user

12/10/2020, 6:26 PM
i 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 PM
but i think it's keeping us honest.
u

user

12/10/2020, 6:27 PM
it'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 PM
Yeah :/
u

user

12/10/2020, 6:33 PM
Feels bad to either have to do something hacky or spend hours to add a trivial test case
u

user

12/10/2020, 6:34 PM
Going to do the hacky one and see if it works
3 Views