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

[#21384] Increase sql package test coverage from 35% to 70% #33711

Merged
merged 4 commits into from
Jan 27, 2025

Conversation

dev-mohit06
Copy link
Contributor

PR Summary:

This PR addresses #21384 by adding comprehensive unit tests to the sql package, increasing the code coverage from 35% to 75%. The following test cases have been added:

  • TestOutputType: Tests output type handling with and without components.
  • TestTransform: Ensures proper validation of required options.
  • TestMultipleOptions: Verifies that multiple options can be applied correctly.

Testing:

  • All tests pass locally.
  • Code coverage has increased from 35% to 75%.
  • Tests adhere to the existing codebase style and conventions.
  • Proper documentation and comments have been added for the test functions.

Checklist:

Additional Notes:

  • The tests focus on configuration and option-setting functionality.
  • The TestTransform tests validate the required options without needing the expansion service.
  • Tests follow the established test patterns in the codebase.

Screenshots:

  • Coverage Output:
    Coverage Output

  • Coverage.out File:
    Coverage Out File

  • Test Cases Passing Locally:
    Test Cases Passing

@github-actions github-actions bot added the go label Jan 22, 2025
Copy link
Contributor

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @lostluck for label go.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

@dev-mohit06 dev-mohit06 changed the title [#21384] Increase sql package test coverage from 35% to 75% [#21384] Increase sql package test coverage from 35% to 70% Jan 22, 2025
@dev-mohit06
Copy link
Contributor Author

@lostluck Can you please review and merge it to the repo?

Copy link
Contributor

@lostluck lostluck left a comment

Choose a reason for hiding this comment

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

Thank you very much Mohit! This is wonderful. I do have some style and consistency comments however. It's a good rule of thumb to not introduce a wildly different style to a codebase within a given language, as it makes it harder to work in.

What you have here is good, but it can be better (though you might need to do a bit of reading first).

sdks/go/pkg/beam/transforms/sql/sql_test.go Outdated Show resolved Hide resolved
sdks/go/pkg/beam/transforms/sql/sql_test.go Outdated Show resolved Hide resolved
sdks/go/pkg/beam/transforms/sql/sql_test.go Outdated Show resolved Hide resolved
sdks/go/pkg/beam/transforms/sql/sql_test.go Outdated Show resolved Hide resolved
@dev-mohit06
Copy link
Contributor Author

@lostluck, thank you for telling me. As I am learning Go, I will improve myself and make the changes you mentioned above.

@github-actions github-actions bot added build and removed build labels Jan 24, 2025
@dev-mohit06
Copy link
Contributor Author

Hey @lostluck, could you take a look at the changes?

@dev-mohit06 dev-mohit06 requested a review from lostluck January 24, 2025 14:08
@dev-mohit06
Copy link
Contributor Author

Hey @lostluck, just following up on this. Could you take a look when you have a chance? Thanks!

Copy link
Contributor

@lostluck lostluck left a comment

Choose a reason for hiding this comment

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

Thank you for your patience. Last week became surprisingly busy, and I kept losing the tab where I had this open.

A few more comments, but your updates are steps in the right direction!

.gitignore Outdated Show resolved Hide resolved
sdks/go/pkg/beam/transforms/sql/sql_test.go Outdated Show resolved Hide resolved
sdks/go/pkg/beam/transforms/sql/sql_test.go Outdated Show resolved Hide resolved
sdks/go/pkg/beam/transforms/sql/sql_test.go Outdated Show resolved Hide resolved
…d pipelines (@dev-mohit06)

- Replaced  with  in  and  for better error reporting and readability.
- Isolated pipeline, scope, and collection creation in  to ensure independent test cases.
- Consolidated checks in  by constructing an  options struct and comparing it with the actual struct using .
- Added a custom comparer for  to ensure accurate comparison of underlying types.
- Removed specific  entry from  as it is unnecessary.
@github-actions github-actions bot added the build label Jan 27, 2025
@dev-mohit06 dev-mohit06 requested a review from lostluck January 27, 2025 21:45
@github-actions github-actions bot removed the build label Jan 27, 2025
Copy link
Contributor

@lostluck lostluck left a comment

Choose a reason for hiding this comment

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

Approved after the .gitignore change is removed.

This looks great now! Thank you for your patience and understanding through the review cycle. I hope you've learned a few things about Go testing here.

Removed the specific `coverage.out` entry from `.gitignore` as it is unnecessary for the project.
@dev-mohit06
Copy link
Contributor Author

Hi @lostluck, Could you please accept my LinkedIn connection request? Here’s my profile for reference: https://www.linkedin.com/in/mohit-paddhariya/

Looking forward to connecting with you!

Best regards,
Mohit

@dev-mohit06
Copy link
Contributor Author

@lostluck, Could you let me know when my branch will be merged?

@lostluck
Copy link
Contributor

As soon as the test suite passes.

Thanks again!

Copy link

codecov bot commented Jan 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.48%. Comparing base (8c4bec8) to head (cf7d63b).
Report is 52 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##             master   #33711   +/-   ##
=========================================
  Coverage     57.47%   57.48%           
  Complexity     1474     1474           
=========================================
  Files           985      985           
  Lines        155895   155910   +15     
  Branches       1076     1076           
=========================================
+ Hits          89598    89618   +20     
  Misses        64076    64076           
+ Partials       2221     2216    -5     
Flag Coverage Δ
go 34.79% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dev-mohit06
Copy link
Contributor Author

As soon as the test suite passes.

Thanks again!

@lostluck - I really appreciate your mentorship and guidance throughout this project. Thank you for taking the time to help me grow professionally. I've learned a lot working with you. If you're comfortable, I would be grateful if you could endorse my skills on https://www.linkedin.com/in/mohit-paddhariya, as it would be valuable for my professional development.

@lostluck lostluck merged commit b3dd0ad into apache:master Jan 27, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add unit tests to the sql package
2 participants