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

test: bigquery mock server #775

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

PrathameshTugaonkar
Copy link

Because

  • Essentially because we could mock the BQ server to test it seamlessly

This commit

  • tried and researched various approaches and added final approach to mock BQ server

@PrathameshTugaonkar PrathameshTugaonkar changed the title Prathamesh tugaonkar/bigquery mock server [test ]Prathamesh tugaonkar/bigquery mock server Oct 23, 2024
@PrathameshTugaonkar PrathameshTugaonkar changed the title [test ]Prathamesh tugaonkar/bigquery mock server test: bigquery mock server Oct 23, 2024
@PrathameshTugaonkar PrathameshTugaonkar marked this pull request as ready for review October 23, 2024 16:19
@chuang8511
Copy link
Member

Hi @PrathameshTugaonkar
Thanks for your contribution.
The mock seems good.

Now, could you

  1. Please pre-commit install, you can check the lint before commit.
  2. Please follow the git guideline. It is in the 3. point of sending PR
  3. Please refer to json operator test code to increase the test code coverage. At least, we need to test
  • func (e *execution) Execute(ctx context.Context, jobs []*base.Job) error {
  • func (c *component) GetDefinition(sysVars map[string]any, compConfig *base.ComponentConfig) (*pb.ComponentDefinition, error) {

Thanks for your contribution again!

@PrathameshTugaonkar
Copy link
Author

Thanks @chuang8511 for reviewing and providing suggestions.

Is it good to go now? @chuang8511

Point 3 - Should we take that in another ticket?

@chuang8511
Copy link
Member

Thanks for your question!

Point 3 - Should we take that in another ticket?

No, the issue in this ticket mainly lacks the test code for bigquery component.
And, the previous bottleneck is we don't find a good way to mock bigquery.
The end goal here is still adding test for bigquery component.

So, we will still need point 3 in this ticket!

@PrathameshTugaonkar
Copy link
Author

Thanks for your question!

Point 3 - Should we take that in another ticket?

No, the issue in this ticket mainly lacks the test code for bigquery component. And, the previous bottleneck is we don't find a good way to mock bigquery. The end goal here is still adding test for bigquery component.

So, we will still need point 3 in this ticket!

Okay, Definitely will work towards it. Thanks @chuang8511 !

@PrathameshTugaonkar
Copy link
Author

@chuang8511 Please review. Thanks!

@chuang8511
Copy link
Member

Hi @PrathameshTugaonkar ,

Thanks for your contribution.

What I mean exactly is - we need to do the test like here.

In your code, you only test bigquery http fake server. But, you don't interact the bigquery component with the bigquery http fake server.

Recently, I happened to build the similar thing. You can also take a look on this as a reference.

Copy link
Member

@chuang8511 chuang8511 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check this comment.

assert.NotNil(t, client)
}

func TestExecute(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to test the interaction between the component and fake bigquery server

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: No status
Development

Successfully merging this pull request may close these issues.

[BigQuery] [GCS] Better ways to deal with google client mock
3 participants