Skip to content

Conversation

@misolt
Copy link
Member

@misolt misolt commented Apr 2, 2025

HiveMetadataConnectorTest#testLoadedhive312 is an integration test in the Dumper's unit test suite.

The test works by running the Dumper 5 times against in-memory metastore, then checking that no exceptions are thrown and the results are not empty.

  • This test doesn't test much of the application logic, it tests integration instead.
  • It adds about 5 minutes to every run of the tests.
  • This test is run conditionally depending on the developer's system, making the tests less reliable overall.

@misolt misolt changed the title Remove an integration test from unit tests [b/404497459] Remove an integration test from unit tests Apr 2, 2025
@misolt misolt marked this pull request as ready for review April 2, 2025 08:53
@vladislav-sidorovich
Copy link
Member

Well, the removing is an option for sure.

But did we expose other options? What is a root cause to running for minutes, why it's not 1-2 seconds (what is really more than enough time)

@misolt
Copy link
Member Author

misolt commented Apr 2, 2025

Well, the removing is an option for sure.

But did we expose other options? What is a root cause to running for minutes, why it's not 1-2 seconds (what is really more than enough time)

I investigated it, of the 5 minutes:

  • It takes abt. 2 to run the Dumper 5 times
  • It takes almost 3 minutes to set up the test, i.e. start the Hive instance

I think 3 minutes is a reasonable amount of time for an integration test, but far too long to be run as one of the project's unit tests, especially if integration tests exist to test the same functionality.

Copy link
Member

@vladislav-sidorovich vladislav-sidorovich left a comment

Choose a reason for hiding this comment

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

As discussed we have it covered with the integration tests internally at Google.

@misolt misolt merged commit f5c603f into google:main Aug 27, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants