Fix `ProvidersTest#network_attributes()` test `TODO` #24

pablo-guardiola · 2024-09-04T20:44:23Z

As I continue my journey exploring capture-sdk's repo / learning how bitdrift's Capture SDK works, I noticed that ProvidersTest had an easy TODO that I could tackle, so this is a quick fix that addresses that tech debt. While working on that, I also went ahead and refactored ProvidersTest and added some more coverage to ClientAttributes, DeviceAttributes and NetworkAttributes.

Fixes ProvidersTest#network_attributes() test TODO:

Removes @Ignore annotation
Refactors ClientAttributes, DeviceAttributes and NetworkAttributes tests out of ProvidersTest and into their respective test classes
Adds more coverage to ClientAttributes, DeviceAttributes and NetworkAttributes
Bumps Robolectric dependency version to 4.13

Refactored the tests to have only 1 assertion per test and one test per scenario 👀 https://speakerdeck.com/guardiola31337/elegant-unit-testing-mobilization-2016?slide=57

Basically, if one of them fails you'll know at a glance what in the code might have caused that failure and in which part (class, method, etc.).

The problems of having multiple assertions are:

It's annoying to have to look at the stacktrace to know what's failing (which assertion failed?)
You don't have information about the remaining assertions (because when an assertion fails it breaks the execution of the test)

I rarely write tests with multiple assertions (at least I try 😅), only if they're tearing apart the result object to verify everything e.g. if there's no equals (which is another problem although could happen if it's coming from a 3rd party library object) or if the assertions are atomic to the object.

This is a technique that I learnt from the book Working Effectively With Unit Tests. I really like the techniques described on it in favor of DAMP (Descriptive And Maintainable Procedures). Mainly because they increase maintainability and readability of tests. Another great one (already followed in the project ❤️) is to structure the tests following the triple A - Arrange / Act / Assert (I removed the explicit comments as IMO are unnecessary).

BTW, I highly recommend you reading that book (if you haven't already). It's 🔝 💯

github-actions · 2024-09-04T20:44:41Z

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

@ignore

- remove @ignore annotation - refactor ClientAttributes, DeviceAttributes and NetworkAttributes tests out of ProvidersTest and into their respective test classes - add more coverage to ClientAttributes, DeviceAttributes and NetworkAttributes - bump robolectric dependency version to 4.13

pablo-guardiola · 2024-09-04T20:50:57Z

I have read the CLA Document and I hereby sign the CLA

murki · 2024-09-04T21:27:14Z

Thanks @pablo-guardiola ! I'll do a quick pass and take a closer look tomorrow

murki · 2024-09-04T21:27:47Z

platform/jvm/capture/src/test/kotlin/io/bitdrift/capture/ProvidersTest.kt

-    }
-
-    @Test
-    @Ignore("Robolectric throwing ClassNotFoundException: android.telephony.TelephonyCallback")


what was the actual fix of the TODO/failure? Was it just bumping Robolectric to 4.13?

That's correct. That issue was fixed in robolectric/robolectric#7786 and released as part of Robolectric 4.10 - it seems that the test was marked as ignored at some point and never revisited ¯\_(ツ)_/¯

That being said, I bumped to the latest available to avoid ClientAttributesTest#package_info_android_tiramisu()'s issue Skip package_info_android_tiramisu because Robolectric doesn't support legacy resources mode after P (which gets fixed using Robolectric 4.11+).

murki · 2024-09-04T21:29:35Z

platform/jvm/gradle/libs.versions.toml

@@ -19,7 +19,7 @@ mockitoCore = "4.9.0"
 mockitoKotlin = "2.2.0"
 mockitoKotlinVersion = "4.1.0"
 okhttp = "4.12.0"
-robolectric = "4.9"


Until we finish the migration from bazel to gradle the integration (robolectric) tests are mostly run via CI using bazel, which means you might need to also bump

capture-sdk/bazel/capture_dependencies.bzl

Line 21 in a5e8d0f

"org.robolectric:robolectric:4.11",

As you can tell the versions were already out of sync :(

Great catch! I tested both Bazel (./bazelw test //platform/jvm/...) and Gradle locally. I was lucky that Robolectric's Bazel version was set to 4.11 and everything was passing 😅 (refs. #24 (comment)). Will bump Bazel dependency version to 4.13 to have them in sync 👍

Will bump Bazel dependency version to 4.13 to have them in sync 👍

Done 👉 b3ac74c

murki · 2024-09-04T21:31:48Z

There might be a bug in our CI checks where we didn't trigger the android jobs cause we didn't detect any "meaningful" android files had changed, will need to take a closer look, or @pablo-guardiola if you're curious this is the check that is managing the CI triggers

capture-sdk/.github/workflows/android.yaml

Line 27 in a5e8d0f

    
                     ./ci/check_bazel.sh //examples/android:android_app || ./ci/files_changed.sh .github/workflows/android.yaml || ./ci/files_changed.sh "^platform/jvm/gradle-test-app/.*\.(gradle|kts|kt|xml)$" && ./ci/run_tests.sh

and then misbehaving here https://github.com/bitdriftlabs/capture-sdk/actions/runs/10709321607/job/29695308132?pr=24#step:3:6

pablo-guardiola · 2024-09-05T15:38:46Z

There might be a bug in our CI checks where we didn't trigger the android jobs cause we didn't detect any "meaningful" android files had changed

Great catch @murki 👍

files_changed script is failing for external contributions. As you pointed out, root cause seems having origin hardcoded 👀

capture-sdk/ci/files_changed.sh

Line 10 in a5e8d0f

    
           git rev-parse --abbrev-ref HEAD | grep -q ^main$ || git diff --name-only "origin/$GITHUB_BASE_REF" | grep -E "$1"

Another great point you brought was that the script is currently implemented in "fail-open" mode rather than "fail-closed". That should be fixed as well to report red if something fails - currently it feels that everything worked as the checks passed.

Will try to take a stab at these and report back here.

murki · 2024-09-06T14:26:57Z

Will try to take a stab at these and report back here.

Thanks @pablo-guardiola, yeah ideally we should make the fixes on this same PR so we can progressively test that they work.

Re: "fail-closed":

Remove the true at the end of the github run step command
Potentially simplify ci/files_changed.sh so it's more explicit. Something like this:

if git rev-parse --abbrev-ref HEAD | grep -q ^main$; then
  exit 0
fi

git diff --name-only "origin/$GITHUB_BASE_REF" | grep -E "$1"

Re: external contributions:

We might need to fiddle with the ci/files_changed.sh script further to make it recognize forks not in origin. The final result could look something like this:

set -euo pipefail

# Fetch the base branch from the original repository using its URL
git fetch "https://github.com/${GITHUB_REPOSITORY}.git" "${GITHUB_BASE_REF}"

# Check if the current branch is `main`, exit if it is
if git rev-parse --abbrev-ref HEAD | grep -q ^main$; then
  exit 0
fi

# Check if relevant files have changed between the current branch and the base branch
git diff --name-only "FETCH_HEAD" | grep -E "$1"

pablo-guardiola · 2024-09-06T16:35:38Z

Hey @murki thanks for the suggestions! They're exactly what I was working on and the same approach I ended up taking.

I've just pushed the changes in d2fc882 Could you please kick-off CI when you have a chance so we can see if everything is working now?

pablo-guardiola · 2024-09-06T16:45:09Z

https://github.com/bitdriftlabs/capture-sdk/actions/runs/10741877498/job/29793722002?pr=24

Run ./ci/check_bazel.sh //examples/android:android_app || ./ci/files_changed.sh .github/workflows/android.yaml || ./ci/files_changed.sh "^platform/jvm/gradle-test-app/.*\.(gradle|kts|kt|xml)$" && ./ci/run_tests.sh
  ./ci/check_bazel.sh //examples/android:android_app || ./ci/files_changed.sh .github/workflows/android.yaml || ./ci/files_changed.sh "^platform/jvm/gradle-test-app/.*\.(gradle|kts|kt|xml)$" && ./ci/run_tests.sh
  shell: /usr/bin/bash -e {0}
fatal: Not a valid object name origin/pg-fix-providers-network-attributes-test
.github/workflows/android.yaml
Tests will run

Looking fatal: Not a valid object name origin/pg-fix-providers-network-attributes-test 🤔

pablo-guardiola · 2024-09-06T17:11:33Z

I think I know what's going on. It seems that we're not fetching the remote base branch. Apparently, even though fetch-depth: 0 does fetch the entire history of the repository, it doesn't automatically fetch all remote branches, so fetching it explicitly may fix it. Let me quickly try that.

pablo-guardiola · 2024-09-06T17:17:39Z

I've just pushed @murki 6ac9cb8

pablo-guardiola · 2024-09-09T17:24:54Z

@murki got some time to implement the "fail-closed" mode properly and now should be working fine (including external contributions / forks) 🚀

Please, give it a try when you have a chance 🙏

murki

Thanks @pablo-guardiola for your contributions and your steadfastness towards improving our CI along the way. Changes look good to me

pablo-guardiola force-pushed the pg-fix-providers-network-attributes-test branch from 75b1f07 to ac6305e Compare September 4, 2024 20:50

mattklein123 requested a review from murki September 4, 2024 21:22

mattklein123 assigned murki Sep 4, 2024

murki reviewed Sep 4, 2024

View reviewed changes

bump robolectric bazel dependency version to 4.13

b3ac74c

pablo-guardiola force-pushed the pg-fix-providers-network-attributes-test branch 3 times, most recently from 118caa5 to d2fc882 Compare September 6, 2024 16:29

implement "fail-closed" mode

f43625b

pablo-guardiola force-pushed the pg-fix-providers-network-attributes-test branch from 79e8341 to f43625b Compare September 9, 2024 17:19

murki approved these changes Sep 9, 2024

View reviewed changes

murki merged commit 224b5e2 into bitdriftlabs:main Sep 9, 2024
15 checks passed

github-actions bot locked and limited conversation to collaborators Sep 9, 2024

pablo-guardiola deleted the pg-fix-providers-network-attributes-test branch September 9, 2024 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `ProvidersTest#network_attributes()` test `TODO` #24

Fix `ProvidersTest#network_attributes()` test `TODO` #24

pablo-guardiola commented Sep 4, 2024

github-actions bot commented Sep 4, 2024 •

edited

Loading

pablo-guardiola commented Sep 4, 2024

murki commented Sep 4, 2024

murki Sep 4, 2024

pablo-guardiola Sep 5, 2024

murki Sep 4, 2024

pablo-guardiola Sep 5, 2024

pablo-guardiola Sep 5, 2024

murki commented Sep 4, 2024 •

edited

Loading

pablo-guardiola commented Sep 5, 2024 •

edited

Loading

murki commented Sep 6, 2024 •

edited

Loading

pablo-guardiola commented Sep 6, 2024

pablo-guardiola commented Sep 6, 2024

pablo-guardiola commented Sep 6, 2024 •

edited

Loading

pablo-guardiola commented Sep 6, 2024

pablo-guardiola commented Sep 9, 2024

murki left a comment

Fix ProvidersTest#network_attributes() test TODO #24

Fix ProvidersTest#network_attributes() test TODO #24

Conversation

pablo-guardiola commented Sep 4, 2024

github-actions bot commented Sep 4, 2024 • edited Loading

pablo-guardiola commented Sep 4, 2024

murki commented Sep 4, 2024

murki Sep 4, 2024

Choose a reason for hiding this comment

pablo-guardiola Sep 5, 2024

Choose a reason for hiding this comment

murki Sep 4, 2024

Choose a reason for hiding this comment

pablo-guardiola Sep 5, 2024

Choose a reason for hiding this comment

pablo-guardiola Sep 5, 2024

Choose a reason for hiding this comment

murki commented Sep 4, 2024 • edited Loading

pablo-guardiola commented Sep 5, 2024 • edited Loading

murki commented Sep 6, 2024 • edited Loading

Re: "fail-closed":

Re: external contributions:

pablo-guardiola commented Sep 6, 2024

pablo-guardiola commented Sep 6, 2024

pablo-guardiola commented Sep 6, 2024 • edited Loading

pablo-guardiola commented Sep 6, 2024

pablo-guardiola commented Sep 9, 2024

murki left a comment

Choose a reason for hiding this comment

Fix `ProvidersTest#network_attributes()` test `TODO` #24

Fix `ProvidersTest#network_attributes()` test `TODO` #24

github-actions bot commented Sep 4, 2024 •

edited

Loading

murki commented Sep 4, 2024 •

edited

Loading

pablo-guardiola commented Sep 5, 2024 •

edited

Loading

murki commented Sep 6, 2024 •

edited

Loading

pablo-guardiola commented Sep 6, 2024 •

edited

Loading