Closed Bug 1622691 Opened 1 year ago Closed 2 months ago

testing/mozbase/rust/mozdevice/src/lib.rs: Fix the clippy warnings

Categories

(Testing :: Mozbase Rust, task, P5)

task

Tracking

(firefox88 fixed)

RESOLVED FIXED
88 Branch
Tracking Status
firefox88 --- fixed

People

(Reporter: Sylvestre, Assigned: me, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, Whiteboard: [lang=rust])

Attachments

(1 file, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #1622690 +++

Filling as a good first bug to learn workflows.

As the change is easy, it is mostly to learn how to contribute to Firefox.

To run the linter:
$ ./mach lint -l clippy testing/mozbase/rust/mozdevice/src/lib.rs --warnings

Warnings:

xxx/testing/mozbase/rust/mozdevice/src/lib.rs
   29:39  warning  trivial regex                                   clippy::trivial_regex (clippy)
   88:44  warning  redundant clone                                 clippy::redundant_clone (clippy)
  278:25  warning  calling `to_string` on `&&std::string::String`  clippy::inefficient_to_string (clippy)

Tutorial to contribute:
https://firefox-source-docs.mozilla.org/tools/docs/contribute/how_to_contribute_firefox.html

Please don't ask for the bug to be assigned. It will be automatically assigned to the first patch.

Fix simple linting errors

Assignee: nobody → me
Status: NEW → ASSIGNED
Attachment #9175364 - Attachment description: Bug 1622691 - Short description of your change. r?Sylvestre → Bug 1622691 - Fix simple linting errors r?Sylvestre

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Assignee: me → nobody
Status: ASSIGNED → NEW

Henrik, Was this ready to land?

Flags: needinfo?(hskupin)

Well it was, but not sure if it still applies. I might expect merge issues.

Given that Sylvetre mentores this bug, I will needinfo him for follow-up, and also move the bug to the correct component.

Assignee: nobody → me
Status: NEW → ASSIGNED
Component: Lint and Formatting → Mozbase Rust
Flags: needinfo?(hskupin) → needinfo?(sledru)
Product: Firefox Build System → Testing

I just run it and it is still valid:

19:02:05.933 clippy (810969) | Finished in 435.81 seconds
/home/sylvestre/dev/mozilla/mozilla-unified/testing/mozbase/rust/mozdevice/src/lib.rs
  33:39  warning  trivial regex                                                                        clippy::trivial_regex (clippy)
  97:18  error    using `to_string` in `fmt::Display` implementation might lead to infinite recursion  clippy::to_string_in_display (clippy)
Flags: needinfo?(sledru)

Ok, great. So when the patch still applies can you land it?

Flags: needinfo?(sledru)

it doesn't apply and not sure why you reassigned to Darrien as the bug didn't move for 5 months

Assignee: me → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(sledru)

The patch was ready to land by that time, but no-one actually landed it. It was outside of our component and as such it wasn't clear that Darrien doesn't have permissions.

Darrien, would you be interested to update the patch? I would appreciate.

Flags: needinfo?(me)

Hey! I don't mind updating the patch. You'll have to give me a few days, I haven't worked on Firefox in a bit. But will get to it shortly!

Flags: needinfo?(me)

Thanks a lot, and yes take your time.

Assignee: nobody → me
Status: NEW → ASSIGNED

Hey folks, I have a new patch up that fixes all of the new clippy errors: https://phabricator.services.mozilla.com/D105539

I'm a little sketchy on the commands to update the previous commit so I made a new patch. That also means the old one can be removed.

Attachment #9175364 - Attachment is obsolete: true
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e1972a4c557a
Fix simple linting errors r=webdriver-reviewers,jgraham

There is a unit test failure:

[task 2021-03-16T14:44:27.335Z] 14:44:27     INFO -  test shell::tests::escape_newline ... FAILED
[task 2021-03-16T14:44:27.335Z] 14:44:27     INFO -  failures:
[task 2021-03-16T14:44:27.335Z] 14:44:27     INFO -  ---- shell::tests::escape_newline stdout ----
[task 2021-03-16T14:44:27.335Z] 14:44:27     INFO -  thread 'shell::tests::escape_newline' panicked at 'assertion failed: `(left == right)`
[task 2021-03-16T14:44:27.335Z] 14:44:27     INFO -    left: `"\\\\n"`,
[task 2021-03-16T14:44:27.335Z] 14:44:27     INFO -   right: `"\\\'\n\'"`', testing/mozbase/rust/mozdevice/src/shell.rs:64:9

There might be something wrong in the logic of the code given that the final replacement for '\n' will never be run. At this time \n has already been converted to \\\\n, so that the pattern doesn't match. But that's not a regression from fixing the clippy warnings, and should be investigated separately.

Attachment #9203835 - Attachment description: Bug 1622691: Fix simple linting errors r?whimboo → Bug 1622691: Fix clippy warnings for mozdevice.
Attachment #9203835 - Attachment description: Bug 1622691: Fix clippy warnings for mozdevice. → Bug 1622691 - [rust-mozdevice] Fix clippy warnings.

I updated the patch to fix the test failure, and triggered its landing.

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b797aca2a45c
[rust-mozdevice] Fix clippy warnings. r=webdriver-reviewers,jgraham
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch

Darrien, I want to thank again for getting this bug fixed! By surprise it even fixed a stack overflow crash. As such we might have to always take care of clippy warnings in the future.

Mentor: sledru → hskupin
Flags: needinfo?(me)

I'm just glad I was able to help fix something with my favorite browser. Thanks for helping this land!

Thank YOU
I didn't realize that this would fix a crash itself :)

Feel free to reach out in case you are interested to work on something else. If yes and it would be Rust, JS, or Python related, and in combination with automation feel free to join us in https://chat.mozilla.org/#/room/#interop:mozilla.org.

I appreciate the guidance. I'm in the process of buying a house right now and am a little busy, but will certainly join afterwards. Thanks again :)

You need to log in before you can comment on or make changes to this bug.