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

[Flaky unit test] Adding file based uri. #12671

Merged
merged 2 commits into from
Jul 11, 2022

Conversation

cryptoe
Copy link
Contributor

@cryptoe cryptoe commented Jun 17, 2022

Description

Fixes the unit test HttpEntity which was contacting https://druid.apache.org/data/wikipedia.json.gz. Changed it in a way that we are using a local URI.

The uber goal was to fix #10853 but that is tricky as it would require a new Nginx docker image to be bundled to serve static contents.
Will defer it until #12368 makes it into master.


Key changed/added classes in this PR
  • HttpEntityTest

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@cryptoe cryptoe mentioned this pull request Jun 17, 2022
9 tasks
@cryptoe cryptoe changed the title Adding file based uri. [Flaky unit test] Adding file based uri. Jun 17, 2022
@kfaraz
Copy link
Contributor

kfaraz commented Jun 21, 2022

The updated test seems to be passing (there is an unrelated failure).

But upon taking another look at the method under test, HttpEntity.openInputStream(), I don't think the updated test adds much value. The openInputStream() method reads the content-range header which makes sense only for an HTTP URL connection, and not a file URI. Also, since the resource file does not actually exist, this is a trivial verification (no content == no content) rather than a verification of the stream skipping content.

Even though I had suggested using a file uri earlier in #12668 , I guess it is not the best approach to fix this test after all.

What do you think @cryptoe?

@cryptoe
Copy link
Contributor Author

cryptoe commented Jun 21, 2022

I am in favor of going via the HTTP route as it's a more robust test. The server approach seemed fine to be as the server is contained in the UT and the cost penalty is not that big.
We are actually matching the content in this case as well. I did not understand your (no content== no content) comment

@gianm
Copy link
Contributor

gianm commented Jun 24, 2022

I agree it'd be better to set up a temporary, in-process HTTP server for this test.

@cryptoe
Copy link
Contributor Author

cryptoe commented Jul 7, 2022

Added the server back.

@abhishekagarwal87 abhishekagarwal87 merged commit cebf2ba into apache:master Jul 11, 2022
@abhishekagarwal87 abhishekagarwal87 added this to the 24.0.0 milestone Aug 26, 2022
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.

The inputSource integration test is flaky
4 participants