Add androidTests for httpUrlConnection module and create test-common module for common test utilities #452

surbhiia · 2024-06-25T16:31:37Z

This PR adds relevant androidTests for httpUrlConnection module.
Test-common module has also been added to park the common test utilities

Related Issue - #148

Feel free to suggest better naming for the new files. One observation I had was that, by naming module "test-common" it moves to the bottom of the list in project view in android studio and if we were to name it "common-test" it would show up right below the "common" module. Let me know if anyone has any preferences here. :)

…ls in test-common module

marandaneto · 2024-06-26T07:08:53Z

...est/java/io/opentelemetry/instrumentation/library/httpurlconnection/InstrumentationTest.java

+        HttpUrlConnectionTestUtil.executeCustomGet("http://httpbin.org/get", false, false);
+        ScheduledExecutorService harvester = scheduleHarvester();
+        try {
+            Thread.sleep(15000);


I'd try to write the test in a way that we don't need to sleep that long, usually, this slows down the test pipeline quite fast

➕ @surbhiia maybe you can somehow mock the time delay instead? We did something like that in the PeriodicRunnableTest class, not sure if the same approach would work though, but probably we can come up with some alternative anyway.

One simpler approach that comes to mind is - when we make connection's inactivity timeout configurable (in next PR), we can set it 0s in this test case and run the harvester just once, right after, to end the trace, we can use ExecutorService and do a 2s awaitTimeout (mostly will finish before that) for this task to complete and do our assertion. Example:

`
ExecutorService executorService = Executors.newSingleThreadExecutor();

try {

executorService.submit(HttpUrlInstrumentationConfig.getReportIdleConnectionRunnable()); executorService.shutdown(); if (executorService.awaitTermination(2, TimeUnit.SECONDS)) { Assert.assertEquals(1, inMemorySpanExporter.finishedSpanItems.size) } else { Assert.fail( "Test could not be completed as tasks did not complete within the 2s timeout period.", ) } } catch (InterruptedException e) { } finally {}

`

We can remove this test for now and add it in next PR (when we make timeout configurable), with this new approach. Does this look good?

Another way to test this is to fake the clock this is using, but it requires a lot of plumbing will be beyond the scope of this change.

I agree with @marandaneto - sleeps not only slow the test down, it can also introduce flakiness down the line because execution in CI is often unpredictable. So if you have to do it, make the value configurable so it won't take that long, and rely on CountDownLatch to verify conditions before proceeding rather than expecting certain things to be done after a certain amount of time.

I've removed the harvester test from this PR. Will add it back in next PR when we make the inactivity timeout configurable, so we can set it to 0 and assert after 1 run of the harvester. I can wrap the runnable with a new runnable with CountDownLatch.

marandaneto · 2024-06-26T07:10:08Z

...est/java/io/opentelemetry/instrumentation/library/httpurlconnection/InstrumentationTest.java

+    private static final InMemorySpanExporter inMemorySpanExporter = InMemorySpanExporter.create();
+
+    @Before
+    public void setUp() {
+        OpenTelemetryTestUtils.setUpSpanExporter(inMemorySpanExporter);
+    }
+
+    @After
+    public void tearDown() {
+        inMemorySpanExporter.reset();


If at some point we enable running tests in parallel, this would be an issue, ideally, we'd have an inMemorySpanExporter per test case or make sure that tests never run in parallel

That's a great point! Will bring it up for discussion in our next Android Sig meeting. :)

not a blocker though, I believe more tests do this (global state).

@marandaneto @LikeTheSalad Do you see any concerns with having an inMemorySpanExporter per test case? We can do that right away to avoid issues later if we were to run tests in parallel? I can make the change in okhttp3.InstrumentationTest.java too for same in this PR.

if tests are not running in parallel, all good, if they do, it is a must to avoid concurrency issues and flaky tests

I would not recommend putting any test infrastructure in static scope. At minimum, have one per test instance, but ideally you create a new one per test run if it involves additional threads. This project isn't so big that reinits are goign to slow the test suite runs down, so it's probably better to be as clean as possible to avoid weird flakiness.

I agree, I think we should avoid statics in general unless it's necessary for a particular test case.

I’ve added a new commit which addresses the following:

Makes a per test instance of inMemorySpanExporter - thereby making it possible to run these tests in parallel

For test purposes only - providing a function for re-setting the instrumenter instance for each test - thereby allowing us to re-set opentelemetry instance (with new exporter) for each test and not depending on GlobalOpentelemetry instance (If tests are run in parallel but in same JVM process, GlobalOpentelemetry instance would be shared, so we can’t use that).

Some other considerations:

For now, for general use case, still depending on GlobalOpentelemetry instance until we have a way around that. (We cannot use a installer like AndroidInstrumentation.kt as we do not want to introduce the requirement of client having to make a manual call to install(opentelemetryInstance) after initializing their OpentelemetryInstance. That adds manual parts to this auto-instrumentation. And if we were to automate this install call, it would be difficult/tedious for auto-instrumentations to figure out where exactly to place this call in client code as it needs to be after their opentelemetryInstance initialization which could vary for each customer.)

For now, I have not used CachedSupplier for getting the instrumenter instance lazily as it synchronizes every read access to instrumenter which I believe has more performance downside. Instead I’ve only synchronized the read access until instrumenter is null and made the variable volatile. We can discuss this more and establish the best approach and unify them.

I can also make the change for okhttp3 androidTests, if this approach seems okay.

...est/java/io/opentelemetry/instrumentation/library/httpurlconnection/InstrumentationTest.java

...va/io/opentelemetry/instrumentation/library/httpurlconnection/HttpUrlConnectionTestUtil.java

marandaneto · 2024-06-26T07:16:34Z

Thanks for adding the tests.
It'd be cool if tests were written in Kotlin already, not a blocker at all though.

surbhiia · 2024-06-26T23:40:25Z

Thanks for adding the tests. It'd be cool if tests were written in Kotlin already, not a blocker at all though.

Will convert this to Kotlin! Not many files in this PR, so should not take long. :)

...est/java/io/opentelemetry/instrumentation/library/httpurlconnection/InstrumentationTest.java

surbhiia · 2024-06-28T20:30:32Z

I'll be on PTO next week. Will address any new comments starting next to next week! :)

bidetofevil · 2024-06-29T18:30:53Z

We talked about wanting all of the code to eventually be Kotlin (I will begin converting soon), so new code, especially tests, should be in Kotlin.

We should add something to the contribution guide about that so anything new is Kotlin unless it really can't be. I can find a place for that.... CC @LikeTheSalad @breedx-splk

LikeTheSalad · 2024-07-01T10:01:47Z

We talked about wanting all of the code to eventually be Kotlin (I will begin converting soon), so new code, especially tests, should be in Kotlin.

We should add something to the contribution guide about that so anything new is Kotlin unless it really can't be. I can find a place for that.... CC @LikeTheSalad @breedx-splk

I agree, we should add it to the contributing guide.

breedx-splk · 2024-07-16T23:52:02Z

...est/kotlin/io/opentelemetry/instrumentation/library/httpurlconnection/InstrumentationTest.kt

+        val inMemorySpanExporter = InMemorySpanExporter.create()
+        HttpUrlConnectionSingletons.setInstrumenterForTesting(OpenTelemetryTestUtils.setUpSpanExporter(inMemorySpanExporter))
+        executeGet("http://httpbin.org/get")
+        Assert.assertEquals(1, inMemorySpanExporter.finishedSpanItems.size)


nit: we should use static imports for these, or use the assertj fluent style of assertThat(...).isEqualTo(1). This applies to all these asserts in this PR.

breedx-splk · 2024-07-16T23:53:48Z

...est/kotlin/io/opentelemetry/instrumentation/library/httpurlconnection/InstrumentationTest.kt

+class InstrumentationTest {
+    @Test
+    fun testHttpUrlConnectionGetRequest_ShouldBeTraced() {
+        val inMemorySpanExporter = InMemorySpanExporter.create()


This inMemorySpanExporter is created at the beginning of every test method and shutdown at the bottom of each. I think this can be better handled by making the exporter a class field and creating it in a @BeforeEach method and shutting it down in the @AfterEach method.

breedx-splk

I appreciate that this increases the tests. Some small room remaining for improvements (more kotlin, etc), but I think it's worth moving forward.

LikeTheSalad · 2024-07-23T07:57:42Z

test-common/src/main/kotlin/io/opentelemetry/android/test/common/OpenTelemetryTestUtils.kt

+import io.opentelemetry.sdk.trace.export.SpanExporter
+
+object OpenTelemetryTestUtils {
+    lateinit var openTelemetry: OpenTelemetry


Maybe after the okhttp inst changes, I think we should update this to avoid relying on storing the otel instance statically for every test. Seems like a test extension/rule could help.

Add androidTests for httpUrlConnection module and pull out common uti…

75f6501

…ls in test-common module

surbhiia requested a review from a team June 25, 2024 16:31

marandaneto reviewed Jun 26, 2024

View reviewed changes

...est/java/io/opentelemetry/instrumentation/library/httpurlconnection/InstrumentationTest.java Outdated Show resolved Hide resolved

marandaneto reviewed Jun 26, 2024

View reviewed changes

...est/java/io/opentelemetry/instrumentation/library/httpurlconnection/InstrumentationTest.java Outdated Show resolved Hide resolved

marandaneto reviewed Jun 26, 2024

View reviewed changes

...va/io/opentelemetry/instrumentation/library/httpurlconnection/HttpUrlConnectionTestUtil.java Outdated Show resolved Hide resolved

Ensure executor.shutdown also gets called finally.

7cd4f4a

marandaneto reviewed Jun 27, 2024

View reviewed changes

...est/java/io/opentelemetry/instrumentation/library/httpurlconnection/InstrumentationTest.java Outdated Show resolved Hide resolved

Convertion to Kotlin

46e3a80

Make tests independent allowing parallel run

a4bc365

breedx-splk reviewed Jul 16, 2024

View reviewed changes

breedx-splk approved these changes Jul 16, 2024

View reviewed changes

LikeTheSalad reviewed Jul 23, 2024

View reviewed changes

LikeTheSalad approved these changes Jul 23, 2024

View reviewed changes

LikeTheSalad mentioned this pull request Jul 23, 2024

Avoid static otel test instance #487

Closed

breedx-splk merged commit fa5eedd into open-telemetry:main Jul 23, 2024
3 checks passed

surbhiia mentioned this pull request Jul 31, 2024

Cleanup httpurlconnection androidTests #503

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add androidTests for httpUrlConnection module and create test-common module for common test utilities #452

Add androidTests for httpUrlConnection module and create test-common module for common test utilities #452

surbhiia commented Jun 25, 2024 •

edited

Loading

marandaneto Jun 26, 2024

LikeTheSalad Jun 26, 2024

surbhiia Jun 27, 2024 •

edited

Loading

bidetofevil Jun 29, 2024

surbhiia Jul 11, 2024

marandaneto Jun 26, 2024

surbhiia Jun 26, 2024

marandaneto Jun 27, 2024

surbhiia Jun 27, 2024 •

edited

Loading

marandaneto Jun 28, 2024

bidetofevil Jun 29, 2024

LikeTheSalad Jul 1, 2024

surbhiia Jul 11, 2024

marandaneto commented Jun 26, 2024

surbhiia commented Jun 26, 2024

surbhiia commented Jun 28, 2024

bidetofevil commented Jun 29, 2024

LikeTheSalad commented Jul 1, 2024

breedx-splk Jul 16, 2024

breedx-splk Jul 16, 2024

breedx-splk left a comment

LikeTheSalad Jul 23, 2024

Add androidTests for httpUrlConnection module and create test-common module for common test utilities #452

Add androidTests for httpUrlConnection module and create test-common module for common test utilities #452

Conversation

surbhiia commented Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surbhiia Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surbhiia Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marandaneto commented Jun 26, 2024

surbhiia commented Jun 26, 2024

surbhiia commented Jun 28, 2024

bidetofevil commented Jun 29, 2024

LikeTheSalad commented Jul 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

breedx-splk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surbhiia commented Jun 25, 2024 •

edited

Loading

surbhiia Jun 27, 2024 •

edited

Loading

surbhiia Jun 27, 2024 •

edited

Loading