libservo: Fix dead code warning and fix two typos in unit tests by Keerti707 · Pull Request #42375 · servo/servo

Keerti707 · 2026-02-05T20:37:56Z

This PR addresses a dead code warning removed during test builds by ensuring the servo test helper constructor is exercised. None of the functional behaviors are changed and this only improves test clarity and keeps the compiler output warning-free.

Fixes: #42364

Keerti707 · 2026-02-05T20:39:52Z

I have opened a PR for this,please check it, I would really appreciate some feedback!

simonwuelker · 2026-02-05T21:07:31Z

Thank you, this looks good!

You just need to sign-off your commits before we can merge this, see https://book.servo.org/contributing/git-setup.html#starting-a-new-change.
You can do this for an existing commit with git commit --amend -s --no-edit.

codecov-commenter · 2026-02-05T21:14:16Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 47.61%. Comparing base (10209cc) to head (48a6fdc).

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #42375      +/-   ##
==========================================
+ Coverage   47.58%   47.61%   +0.02%     
==========================================
  Files         296      296              
  Lines       70633    70633              
  Branches    70633    70633              
==========================================
+ Hits        33614    33632      +18     
+ Misses      35585    35565      -20     
- Partials     1434     1436       +2

Flag	Coverage Δ
unittests	`47.61% <ø> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Keerti707 · 2026-02-05T21:21:27Z

Thank you, this looks good!

You just need to sign-off your commits before we can merge this, see https://book.servo.org/contributing/git-setup.html#starting-a-new-change. You can do this for an existing commit with git commit --amend -s --no-edit.

thank you! I have now tried signing off my commits,I hope its working that way too!

simonwuelker · 2026-02-05T21:24:47Z

Hmm your commits are not signed. You should see a line like Signed-off by: XYZat the end of the commit message.
It also looks like the change doesn't compile (see the failing CI job).

Loirooriol · 2026-02-05T23:02:54Z

I have edited the message to use Fixes: #42364, this links the PR to the issue.

Your commit is still unsigned.

shubhamg13

If you really want to deal with this error. Add #[expect(dead_code)] to this.

shubhamg13 · 2026-02-06T01:34:18Z

components/servo/tests/common/mod.rs

 }

 impl ServoTest {
-    pub(crate) fn new() -> Self {


This function is used by other tests in webview.rs

components/servo/tests/common/mod.rs

shubhamg13 · 2026-02-06T09:34:08Z

components/servo/tests/common/mod.rs

+    #[expect(dead_code)] // Used by some tests and not others
    pub(crate) fn new() -> Self {
-        Self::new_with_builder(|builder| builder)
+        Self::new_with_builder(|b| b)


nit: please revert this

okay! thank you for the suggestion.
Really appreciate it!

shubhamg13

Minor nits, rest LGTM.

Keerti707 · 2026-02-06T10:17:35Z

Hi! 👋
I have updated the PR :
I initially added a zero-argument ServoTest::new() constructor with #[expect(dead_code)] to fix the compile errors in tests but as per the given suggestion i reverted it and i have now done evrything that i could see from my end
All changes are now DCO-signed (Signed-off-by: Keerti Gupta [email protected]).
The branch should now compile cleanly, and the DCO check passes. Please let me know if anything else needs adjustment. Thank you!

github-actions · 2026-02-06T10:25:13Z

🔨 Triggering try run (#21747201717) for Linux (Coverage)

mrobinson

@Keerti707 I think you accidentally removed your fixes for the dead code and left the unrelated changes. Please take another look.

mrobinson · 2026-02-06T10:24:50Z

components/servo/tests/common/mod.rs

 impl ServoTest {
    pub(crate) fn new() -> Self {
-        Self::new_with_builder(|builder| builder)
+        Self::new_with_builder(|b| b)


Looks like this one still needs to be reverted.

Thank you for pointing that out..you are right I reverted the unrelated builder rename,I have now fixed that.

Now the fixes for the dead code are made too!
I am sorry for the confusion I caused by mistake
Please take a look at the PR now!
I hope it works correctly as per the need.

github-actions · 2026-02-06T10:35:23Z

✨ Try run (#21747201717) succeeded.

Signed-off-by: Keerti Gupta this side!! <[email protected]>

Add dead code expectation for ServoTest::new function. Signed-off-by: Keerti Gupta this side!! <[email protected]>

This PR addresses a dead code warning removed during test builds by ensuring the servo test helper constructor is exercised. None of the functional behaviors are changed and this only improves test clarity and keeps the compiler output warning-free. Fixes: #42364 --------- Signed-off-by: Keerti Gupta this side!! <[email protected]> Signed-off-by: Keerti Gupta <[email protected]>

utilities and also the event handling Signed-off-by: Keerti Gupta <[email protected]>

…ompile errors in tests Signed-off-by: Keerti Gupta <[email protected]>

Remove unused dead code expectation and clean up formatting. Signed-off-by: Keerti Gupta this side!! <[email protected]>

Loirooriol · 2026-02-06T18:28:35Z

components/servo/tests/common/mod.rs

 }

 impl ServoTest {
+    #[expect(dead_code)] // Used by some tests and not others


If it's sometimes used, it doesn't make sense to expect it to be dead.
Now I'm getting

warning: this lint expectation is unfulfilled --> components/servo/tests/common/mod.rs:25:14 | 25 | #[expect(dead_code)] // Used by some tests and not others | ^^^^^^^^^ | = note: `#[warn(unfulfilled_lint_expectations)]` on by default

So please use allow(dead_code) instead.

Thank you for noticing that!
I have made the necessary changes!
I hope its working now.

This PR already landed, you need to open a new one

servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Feb 5, 2026

shubhamg13 reviewed Feb 6, 2026

View reviewed changes

Keerti707 force-pushed the fix-servo-test-warning branch from ae60269 to bfb3878 Compare February 6, 2026 08:40

shubhamg13 reviewed Feb 6, 2026

View reviewed changes

components/servo/tests/common/mod.rs Outdated Show resolved Hide resolved

shubhamg13 reviewed Feb 6, 2026

View reviewed changes

Keerti707 force-pushed the fix-servo-test-warning branch from df9af48 to 4970555 Compare February 6, 2026 09:56

shubhamg13 added the T-coverage Do a try run with test coverage label Feb 6, 2026

github-actions bot removed the T-coverage Do a try run with test coverage label Feb 6, 2026

mrobinson reviewed Feb 6, 2026

View reviewed changes

Keerti707 added 2 commits February 6, 2026 17:03

Rename builder parameter for clarity

4a286f1

Signed-off-by: Keerti Gupta this side!! <[email protected]>

Add #[expect(dead_code)] to ServoTest::new

116efc5

Add dead code expectation for ServoTest::new function. Signed-off-by: Keerti Gupta this side!! <[email protected]>

mrobinson changed the title ~~Removing dead code warning for servo test helper tests to improve servo test~~ libservo: Fix dead code warning and fix two typos in unit tests Feb 6, 2026

mrobinson approved these changes Feb 6, 2026

View reviewed changes

servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Feb 6, 2026

mrobinson enabled auto-merge February 6, 2026 12:02

mrobinson added this pull request to the merge queue Feb 6, 2026

servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Feb 6, 2026

Merged via the queue into servo:main with commit 4c2327e Feb 6, 2026
31 checks passed

servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Feb 6, 2026

Keerti707 added 3 commits February 6, 2026 20:53

tests: improves servo test

153be77

utilities and also the event handling Signed-off-by: Keerti Gupta <[email protected]>

Add zero-argument ServoTest::new() with #[expect(dead_code)] to fix c…

4970555

…ompile errors in tests Signed-off-by: Keerti Gupta <[email protected]>

Refactor ServoTest by removing dead code attribute

48a6fdc

Remove unused dead code expectation and clean up formatting. Signed-off-by: Keerti Gupta this side!! <[email protected]>

Loirooriol reviewed Feb 6, 2026

View reviewed changes

Uh oh!

Conversation

Keerti707 commented Feb 5, 2026 • edited by mrobinson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Keerti707 commented Feb 5, 2026

Uh oh!

simonwuelker commented Feb 5, 2026

Uh oh!

codecov-commenter commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Keerti707 commented Feb 5, 2026

Uh oh!

simonwuelker commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Loirooriol commented Feb 5, 2026

Uh oh!

shubhamg13 left a comment • edited by simonwuelker Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shubhamg13 left a comment

Choose a reason for hiding this comment

Uh oh!

Keerti707 commented Feb 6, 2026

Uh oh!

github-actions bot commented Feb 6, 2026

Uh oh!

mrobinson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Keerti707 Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 6, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Keerti707 commented Feb 5, 2026 •

edited by mrobinson

Loading

codecov-commenter commented Feb 5, 2026 •

edited

Loading

simonwuelker commented Feb 5, 2026 •

edited

Loading

shubhamg13 left a comment •

edited by simonwuelker

Loading

Keerti707 Feb 6, 2026 •

edited

Loading