Skip to content

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

Merged
abhishekagarwal87 merged 2 commits intoapache:masterfrom
cryptoe:http_entity_remote
Jul 11, 2022
Merged

[Flaky unit test] Adding file based uri.#12671
abhishekagarwal87 merged 2 commits intoapache:masterfrom
cryptoe:http_entity_remote

Conversation

@cryptoe
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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