Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test coverage for blackhole connector #18482

Open
vlad-lyutenko opened this issue Aug 1, 2023 · 6 comments
Open

Add test coverage for blackhole connector #18482

vlad-lyutenko opened this issue Aug 1, 2023 · 6 comments
Assignees
Labels
good first issue Good for newcomers test

Comments

@vlad-lyutenko
Copy link
Contributor

vlad-lyutenko commented Aug 1, 2023

During work on this PR #18016, noticed that test coverage is pretty small.
I think we could refactor current Smoke test to be more consistent with BaseConnectorSmokeTest.
And think about BaseConnectorTest

@Praveen2112 Praveen2112 added good first issue Good for newcomers test labels Aug 1, 2023
@SakshamGairola
Copy link

Hi @vlad-lyutenko I would like to contribute to this project. Can you assign this ticket me?

@vlad-lyutenko
Copy link
Contributor Author

vlad-lyutenko commented Aug 21, 2023

Thanks for working on this, please notice we are going to remove materialized view support for this connector (#18628), but anyway it's good idea to add bigger test coverage!

@tsafacjo
Copy link

tsafacjo commented Nov 1, 2023

still on going ?
Can I work on it too ?

@vlad-lyutenko
Copy link
Contributor Author

Can I work on it too ?

Sure, thanks!

@ritishalaungani
Copy link

Hi @vlad-lyutenko, I have been taking a look at this ticket, and had a few questions I hope you can answer-

  1. Is the goal of this ticket to get BlackHoleConnectorSmoke test to extend from BaseConnectorSmokeTest?
  2. If yes, then based on my investigation, this doesn't seem possible. Most of the BaseConnectorSmokeTest methods run various queries on the datasource that assume that the datasource is actually retaining data, which is not the case for the BlackHoleConnector. The assertions would fail because the SELECT queries all return NULL values. Am I thinking about this correctly?
  3. Looking through the existing tests in TestBlackHoleSmoke, the coverage seems reasonable to me. Were there any specific queries you found missing and thought should be added to it?

@vlad-lyutenko
Copy link
Contributor Author

Hi @ritishalaungani,
big thx for your interest to this task.
Maybe some description - I was working on some task connected to Materialised Views and I added some support for MV to blackhole connector, but then I realised, that we need test coverage for MV in blackhole connector.
So that's why this task appeared.
But then later we decided to remove support for MV views in this connector, but task left as it is.
So I think if you think it's enough, we can close it.

But maybe @Praveen2112 have some ideas, how to improve this test coverage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers test
Development

No branches or pull requests

5 participants