🧪 [testing improvement] Add tests for AdBlocker#268
Conversation
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @sheepdestroyer, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
📝 WalkthroughWalkthroughMultiple changes: new Robolectric/JUnit tests, introduce MaterialWebView and migrate cache/web clients and WebFragment, update ad-block interception, modernize Nav FAB vibration/gesture APIs, migrate preference init hooks, precompile regex in UserServicesClient, refactor Algolia time logic, require battery-not-low for sync jobs and remove connectivity receiver, plus build/config updates. ChangesAdBlocker Test Coverage
Sequence Diagram(s)(Skipped — changes are broad and include many independent flows.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Kotlin |  |
May 20, 2026 7:58p.m. | Review ↗ |
| Java |  |
May 20, 2026 7:58p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 28 |
| Duplication | -3 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/test/java/io/github/sheepdestroyer/materialisheep/AdBlockerTest.java (1)
52-60: ⚡ Quick winAdd an explicit null-input contract test for
isAd().This block documents null behavior in comments but does not assert it. Please add a dedicated test so null handling is intentionally locked (either
assertThrowsor gracefulfalse, whichever is the intended contract).✅ Suggested test addition
+import static org.junit.Assert.assertThrows; ... + `@Test` + public void testIsAd_NullUrl() { + assertThrows(NullPointerException.class, () -> AdBlocker.isAd(null)); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/test/java/io/github/sheepdestroyer/materialisheep/AdBlockerTest.java` around lines 52 - 60, Add an explicit null-input unit test for AdBlocker.isAd to lock the contract: create a new test (e.g., testIsAd_NullUrl) that calls AdBlocker.isAd(null) and asserts the intended behavior—either assertThrows(NullPointerException.class, () -> AdBlocker.isAd(null)) if NPE is intended, or assertFalse(AdBlocker.isAd(null)) if the method should handle null gracefully; place the new assertion alongside the existing testIsAd_InvalidUrl to make the null contract explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/src/test/java/io/github/sheepdestroyer/materialisheep/AdBlockerTest.java`:
- Around line 69-78: The current test in AdBlockerTest uses
BufferedReader.readLine() which strips newlines and can incorrectly treat a
newline-only body as empty; instead read and assert EOF directly from the raw
InputStream returned by response.getData() (or read all bytes and assert length
== 0) to ensure no bytes are present. Locate the test block referencing
response.getData() and replace the BufferedReader/readLine loop with a direct
check on response.getData().read() returning -1 (or readAllBytes().length == 0)
so newline-only content will fail the empty-body assertion.
---
Nitpick comments:
In
`@app/src/test/java/io/github/sheepdestroyer/materialisheep/AdBlockerTest.java`:
- Around line 52-60: Add an explicit null-input unit test for AdBlocker.isAd to
lock the contract: create a new test (e.g., testIsAd_NullUrl) that calls
AdBlocker.isAd(null) and asserts the intended behavior—either
assertThrows(NullPointerException.class, () -> AdBlocker.isAd(null)) if NPE is
intended, or assertFalse(AdBlocker.isAd(null)) if the method should handle null
gracefully; place the new assertion alongside the existing testIsAd_InvalidUrl
to make the null contract explicit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02fcf819-26e9-4561-add4-9a9eac06e5e9
📒 Files selected for processing (1)
app/src/test/java/io/github/sheepdestroyer/materialisheep/AdBlockerTest.java
| // Read the input stream to verify it's empty | ||
| StringBuilder sb = new StringBuilder(); | ||
| try (BufferedReader reader = | ||
| new BufferedReader(new InputStreamReader(response.getData(), StandardCharsets.UTF_8))) { | ||
| String line; | ||
| while ((line = reader.readLine()) != null) { | ||
| sb.append(line); | ||
| } | ||
| } | ||
| assertEquals("", sb.toString()); |
There was a problem hiding this comment.
Current empty-body check can miss newline-only responses.
Using readLine() strips line breaks, so a body like "\n" would still pass as empty. Assert EOF directly on the raw stream to avoid false positives.
🔧 More robust empty-stream assertion
- // Read the input stream to verify it's empty
- StringBuilder sb = new StringBuilder();
- try (BufferedReader reader =
- new BufferedReader(new InputStreamReader(response.getData(), StandardCharsets.UTF_8))) {
- String line;
- while ((line = reader.readLine()) != null) {
- sb.append(line);
- }
- }
- assertEquals("", sb.toString());
+ try (var input = response.getData()) {
+ assertEquals(-1, input.read());
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Read the input stream to verify it's empty | |
| StringBuilder sb = new StringBuilder(); | |
| try (BufferedReader reader = | |
| new BufferedReader(new InputStreamReader(response.getData(), StandardCharsets.UTF_8))) { | |
| String line; | |
| while ((line = reader.readLine()) != null) { | |
| sb.append(line); | |
| } | |
| } | |
| assertEquals("", sb.toString()); | |
| try (var input = response.getData()) { | |
| assertEquals(-1, input.read()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/test/java/io/github/sheepdestroyer/materialisheep/AdBlockerTest.java`
around lines 69 - 78, The current test in AdBlockerTest uses
BufferedReader.readLine() which strips newlines and can incorrectly treat a
newline-only body as empty; instead read and assert EOF directly from the raw
InputStream returned by response.getData() (or read all bytes and assert length
== 0) to ensure no bytes are present. Locate the test block referencing
response.getData() and replace the BufferedReader/readLine loop with a direct
check on response.getData().read() returning -1 (or readAllBytes().length == 0)
so newline-only content will fail the empty-body assertion.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new unit test suite, AdBlockerTest.java, to verify the functionality of the AdBlocker utility, including domain matching and resource creation. The reviewer recommended enhancing the robustness of the isAd method by adding explicit test cases and handling for null inputs to prevent potential NullPointerException crashes.
| // HttpUrl.parse returns null for null strings, so let's verify AdBlocker handles it gracefully | ||
| // (or throws NPE if intended). | ||
| // Since the current code has HttpUrl.parse(url) and does not do null checks on url initially, | ||
| // passing null throws NullPointerException in HttpUrl.parse. | ||
| // We will just test empty and invalid formats here. | ||
| } | ||
|
|
||
| @Test | ||
| public void testCreateEmptyResource() throws Exception { |
There was a problem hiding this comment.
You've correctly identified a potential NullPointerException if a null URL is passed to AdBlocker.isAd(). Instead of just leaving a comment, it's better to add a test case that explicitly covers this scenario. This makes the test suite more comprehensive and highlights the need for AdBlocker.isAd() to be more robust.
A NullPointerException in AdBlocker.isAd() could lead to uncaught exceptions in AdBlockWebViewClient, potentially crashing the application's web view component.
I suggest splitting this test into two: one for invalid URL formats and another for a null URL. The test for the null URL should assert that false is returned, which is the expected behavior for a robust utility method. This test will fail initially, guiding the necessary fix in `AdBlocker.isAd().
@Test
public void testIsAd_InvalidUrl() {
assertFalse(AdBlocker.isAd(""));
assertFalse(AdBlocker.isAd("not a valid url"));
}
@Test
public void testIsAd_NullUrl() {
// AdBlocker.isAd() should handle null input gracefully instead of crashing.
// This test will fail until the method is updated to check for null.
assertFalse(AdBlocker.isAd(null));
}
|
Reviewed by Hermes ✅ APPROVED PR #268: 🧪 [testing improvement] Add tests for AdBlocker Changes look good:
The test file includes:
This addresses the testing gap for the core ad-blocking logic. The tests use RobolectricTestRunner and Schedulers.trampoline() for synchronous execution, which is appropriate. Code quality:
Ready to merge! |
|
@jules address all relevant comments and reviews. |
|
Reviewed by Hermes ✅ APPROVED This PR adds comprehensive unit tests for the AdBlocker functionality. The test suite covers:
The tests use RobolectricTestRunner with trampoline scheduler for synchronous execution, which is appropriate for Android unit tests. Minor suggestions:
These are minor suggestions and do not block merging. The test suite significantly improves coverage and prevents regressions in the ad-blocking logic. ✅ APPROVED |
Hermes Agent ReviewVerdict: Changes Requested Issues Found
Suggestions
Reviewed by Hermes |
|
@jules — This PR has merge conflicts (DIRTY). Please rebase on main and resolve conflicts. Let me know when fixed. |
I have successfully rebased the branch on |
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.deepsource.toml:
- Around line 5-7: Update the test_patterns value in .deepsource.toml so it
matches this repository's Android test directories (replace the current
test_patterns = ["tests/**"]); include patterns like app/src/test/** and
app/src/androidTest/** (and any other module paths, e.g., */src/test/**) so
DeepSource correctly detects unit and instrumentation tests; edit the
test_patterns entry to add these Android-specific globs and save the file.
In `@app/src/main/java/io/github/sheepdestroyer/materialisheep/WebFragment.java`:
- Around line 349-359: The PDF callback implementations of
PdfAndroidJavascriptBridge.Callbacks (methods onFailure/onLoad/onSuccess) must
be lifecycle-safe: before calling offerExternalApp(), setProgress(),
reloadUrl(), or accessing mWebView/getActivity(), check fragment state (e.g.,
isAdded() && !isRemoving() && getActivity() != null) and that mWebView != null
(and not destroyed) to avoid dereferencing after detach; update the onFailure
and onLoad/onSuccess handlers in WebFragment to early-return if the fragment is
not added or mWebView is null, and apply the same guard pattern to the other PDF
callback blocks referenced (the other PdfAndroidJavascriptBridge.Callbacks
implementations) so all PDF paths mirror the readability/item callbacks'
lifecycle checks.
- Around line 497-509: The MIME-type check in the mWebView.setDownloadListener
lambda can throw NPE when mimetype is null; change the guard to safely compare
(e.g., use PDF_MIME_TYPE.equals(mimetype) or check mimetype != null) before
deciding to setProgress(10), set mIsPdf = true and call
downloadFileAndRenderPdf(), otherwise call offerExternalApp(); update the lambda
around mWebView.setDownloadListener to use this null-safe comparison so null
MIME types fall back to the external-app flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49b346d5-0c69-4d00-aa51-d181fb94006e
📒 Files selected for processing (26)
.deepsource.toml.jules/bolt.mdapp/build.gradleapp/src/main/AndroidManifest.xmlapp/src/main/java/io/github/sheepdestroyer/materialisheep/ComposeActivity.javaapp/src/main/java/io/github/sheepdestroyer/materialisheep/ThemedActivity.javaapp/src/main/java/io/github/sheepdestroyer/materialisheep/UserActivity.javaapp/src/main/java/io/github/sheepdestroyer/materialisheep/WebFragment.javaapp/src/main/java/io/github/sheepdestroyer/materialisheep/accounts/UserServicesClient.javaapp/src/main/java/io/github/sheepdestroyer/materialisheep/data/AlgoliaPopularClient.javaapp/src/main/java/io/github/sheepdestroyer/materialisheep/data/ItemSyncWifiReceiver.javaapp/src/main/java/io/github/sheepdestroyer/materialisheep/data/SyncDelegate.javaapp/src/main/java/io/github/sheepdestroyer/materialisheep/preference/SpinnerPreference.javaapp/src/main/java/io/github/sheepdestroyer/materialisheep/preference/ThemePreference.javaapp/src/main/java/io/github/sheepdestroyer/materialisheep/widget/AdBlockWebViewClient.javaapp/src/main/java/io/github/sheepdestroyer/materialisheep/widget/CacheableWebView.javaapp/src/main/java/io/github/sheepdestroyer/materialisheep/widget/MaterialWebView.javaapp/src/main/java/io/github/sheepdestroyer/materialisheep/widget/NavFloatingActionButton.javaapp/src/main/java/io/github/sheepdestroyer/materialisheep/widget/WebView.javaapp/src/main/res/layout/activity_drawer.xmlapp/src/main/res/layout/activity_login.xmlapp/src/main/res/layout/activity_settings.xmlapp/src/test/java/io/github/sheepdestroyer/materialisheep/AdBlockerTest.javaapp/src/test/java/io/github/sheepdestroyer/materialisheep/AppUtilsTest.javaapp/src/test/java/io/github/sheepdestroyer/materialisheep/PreferencesTest.javabuild.gradle
💤 Files with no reviewable changes (4)
- app/src/main/java/io/github/sheepdestroyer/materialisheep/widget/WebView.java
- app/src/main/java/io/github/sheepdestroyer/materialisheep/data/ItemSyncWifiReceiver.java
- app/src/main/res/layout/activity_drawer.xml
- app/src/main/AndroidManifest.xml
✅ Files skipped from review due to trivial changes (4)
- app/src/main/res/layout/activity_settings.xml
- build.gradle
- .jules/bolt.md
- app/src/main/res/layout/activity_login.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/test/java/io/github/sheepdestroyer/materialisheep/AdBlockerTest.java
| test_patterns = [ | ||
| "tests/**" | ||
| ] |
There was a problem hiding this comment.
Update test path patterns to match actual Android test directories.
test_patterns = ["tests/**"] does not match the current layout used in this repo (for example app/src/test/**), so test files may be misclassified.
Suggested config adjustment
test_patterns = [
- "tests/**"
+ "tests/**",
+ "app/src/test/**",
+ "app/src/androidTest/**"
]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test_patterns = [ | |
| "tests/**" | |
| ] | |
| test_patterns = [ | |
| "tests/**", | |
| "app/src/test/**", | |
| "app/src/androidTest/**" | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.deepsource.toml around lines 5 - 7, Update the test_patterns value in
.deepsource.toml so it matches this repository's Android test directories
(replace the current test_patterns = ["tests/**"]); include patterns like
app/src/test/** and app/src/androidTest/** (and any other module paths, e.g.,
*/src/test/**) so DeepSource correctly detects unit and instrumentation tests;
edit the test_patterns entry to add these Android-specific globs and save the
file.
| new PdfAndroidJavascriptBridge.Callbacks() { | ||
| @Override | ||
| public void onFailure() { | ||
| offerExternalApp(); | ||
| setProgress(100); | ||
| } | ||
|
|
||
| private void setUpWebControls(View view) { | ||
| view.findViewById(R.id.toolbar_web).setOnClickListener(v -> scrollToTop()); | ||
| view.findViewById(R.id.button_back).setOnClickListener(v -> mWebView.goBack()); | ||
| view.findViewById(R.id.button_forward).setOnClickListener(v -> mWebView.goForward()); | ||
| view.findViewById(R.id.button_clear).setOnClickListener(v -> { | ||
| mSystemUiHelper.setFullscreen(true); | ||
| reset(); | ||
| mControls.showNext(); | ||
| }); | ||
| view.findViewById(R.id.button_find).setOnClickListener(v -> { | ||
| mEditText.requestFocus(); | ||
| toggleSoftKeyboard(true); | ||
| mControls.showNext(); | ||
| }); | ||
| mButtonRefresh.setOnClickListener(v -> { | ||
| if (mWebView.getProgress() < 100) { | ||
| mWebView.stopLoading(); | ||
| } else { | ||
| mWebView.reload(); | ||
| } | ||
| @Override | ||
| public void onLoad() { | ||
| setProgress(100); | ||
| } |
There was a problem hiding this comment.
Make the PDF callbacks lifecycle-safe.
These callbacks can finish after the fragment is detached. In that state, onFailure() reaches offerExternalApp() and dereferences getActivity(), while onSuccess() can call back into reloadUrl() on a destroyed mWebView. The readability/item callbacks already guard against this, so the PDF path should do the same.
Suggested fix
private void offerExternalApp() {
+ if (!isAdded() || getActivity() == null || getView() == null) {
+ return;
+ }
final Intent intent = new Intent(Intent.ACTION_VIEW, Uri.parse(mItem.getUrl()));
if (intent.resolveActivity(getActivity().getPackageManager()) == null) {
return;
}
@@
`@Override`
public void onFailure() {
+ if (!isAdded() || getView() == null) {
+ return;
+ }
offerExternalApp();
setProgress(100);
}
`@Override`
public void onLoad() {
+ if (!isAdded() || getView() == null) {
+ return;
+ }
setProgress(100);
}
});
@@
`@Override`
public void onFailure(Call call, IOException e) {
- offerExternalApp();
+ new Handler(Looper.getMainLooper())
+ .post(
+ () -> {
+ if (!isAdded() || getView() == null) {
+ return;
+ }
+ offerExternalApp();
+ });
}
`@Override`
public void onSuccess(String filePath) {
- reloadUrl(PDF_LOADER_URL, filePath);
+ new Handler(Looper.getMainLooper())
+ .post(
+ () -> {
+ if (!isAdded() || getView() == null) {
+ return;
+ }
+ reloadUrl(PDF_LOADER_URL, filePath);
+ });
}
});
}Also applies to: 513-521, 651-665
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/io/github/sheepdestroyer/materialisheep/WebFragment.java`
around lines 349 - 359, The PDF callback implementations of
PdfAndroidJavascriptBridge.Callbacks (methods onFailure/onLoad/onSuccess) must
be lifecycle-safe: before calling offerExternalApp(), setProgress(),
reloadUrl(), or accessing mWebView/getActivity(), check fragment state (e.g.,
isAdded() && !isRemoving() && getActivity() != null) and that mWebView != null
(and not destroyed) to avoid dereferencing after detach; update the onFailure
and onLoad/onSuccess handlers in WebFragment to early-return if the fragment is
not added or mWebView is null, and apply the same guard pattern to the other PDF
callback blocks referenced (the other PdfAndroidJavascriptBridge.Callbacks
implementations) so all PDF paths mirror the readability/item callbacks'
lifecycle checks.
| mWebView.setDownloadListener( | ||
| (url, userAgent, contentDisposition, mimetype, contentLength) -> { | ||
| if (getActivity() == null) { | ||
| return; | ||
| } | ||
| mExternalRequired = true; | ||
| mWebView.setVisibility(GONE); | ||
| getActivity().findViewById(R.id.empty).setVisibility(VISIBLE); | ||
| getActivity().findViewById(R.id.download_button).setOnClickListener(v -> startActivity(intent)); | ||
| } | ||
|
|
||
| private void setProgress(int progress) { | ||
| mProgressBar.setProgress(progress); | ||
| mProgressBar.setVisibility(progress == 100 ? GONE : VISIBLE); | ||
| mButtonRefresh | ||
| .setImageResource(progress == 100 ? R.drawable.ic_refresh_white_24dp : R.drawable.ic_clear_white_24dp); | ||
| } | ||
| } | ||
| if (mimetype.equals(PDF_MIME_TYPE)) { | ||
| setProgress(10); | ||
| mIsPdf = true; | ||
| downloadFileAndRenderPdf(); | ||
| } else { | ||
| offerExternalApp(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Guard the MIME-type check against null.
DownloadListener can hand back a null MIME type. mimetype.equals(PDF_MIME_TYPE) will crash this path instead of falling back to the external-app flow.
Suggested fix
- if (mimetype.equals(PDF_MIME_TYPE)) {
+ if (PDF_MIME_TYPE.equals(mimetype)) {
setProgress(10);
mIsPdf = true;
downloadFileAndRenderPdf();
} else {
offerExternalApp();
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/io/github/sheepdestroyer/materialisheep/WebFragment.java`
around lines 497 - 509, The MIME-type check in the mWebView.setDownloadListener
lambda can throw NPE when mimetype is null; change the guard to safely compare
(e.g., use PDF_MIME_TYPE.equals(mimetype) or check mimetype != null) before
deciding to setProgress(10), set mIsPdf = true and call
downloadFileAndRenderPdf(), otherwise call offerExternalApp(); update the lambda
around mWebView.setDownloadListener to use this null-safe comparison so null
MIME types fall back to the external-app flow.
Hermes Agent ReviewVerdict: Approved — LGTM! SummaryThis PR adds comprehensive tests for the AdBlocker functionality and makes several code quality improvements: Testing Improvements:
Code Quality Improvements:
Security & Performance:
The changes are well-tested, follow modern Android practices, and improve both test coverage and code quality. The PR addresses the stated goal of adding tests for AdBlocker.java and includes additional cleanup that improves the codebase. Reviewed by Hermes |
Hermes Agent ReviewVerdict: Changes Requested Issues Found
Suggestions
Reviewed by Hermes |
|
@jules — Please address all unresolved review comments above. Let us know when fixes are pushed. |
|
@jules — This PR has merge conflicts (DIRTY). Please rebase on main and resolve conflicts. Let me know when fixed. |
I have successfully rebased the branch, resolved all conflicts, pushed the latest fixes, and verified everything passes. I couldn't see any other unresolved inline comments provided to me. Please let me know if there's anything else! |
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 28 |
| Duplication | -3 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
1 participant
🧪 [testing improvement] Add comprehensive tests for AdBlocker.java
AdBlocker.javawhich parses and evaluates URLs against a blocked hosts list.101com.com)sub.101com.com)google.com)WebResourceResponseviacreateEmptyResource().isAd(),isAdHost(), andcreateEmptyResource()now have full unit test coverage powered byRobolectricTestRunnerandSchedulers.trampoline(), preventing any logic regressions when resolving host names from URLs.PR created automatically by Jules for task 6003759647253790886 started by @sheepdestroyer
Summary by CodeRabbit