1647260 - test_gen_feature_definitions.py: method already defined line 85 E0102 (pylint)

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

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

toolkit/components/featuregates/test/python/test_gen_feature_definitions.py
  93:4  error  method already defined line 85  E0102 (pylint)

We should probably remove one of the two

To reproduce :

$  ./mach lint -l pylint -n toolkit/components/featuregates/test/python/test_gen_feature_definitions.py

As the change should be easy, it is just to learn how to contribute to Firefox.

Tutorial to contribute:
https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html

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

Comment 1

[Taegeon Lee]

Attachment: Bug 1647260 Remove duplicated 'test_str_with_file' method. r=Sylvestre

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Oh, please keep it :)
I was just talking about future commits

Comment 3

[Kanishk]

Attachment: Bug 1647260 - removed the method previously on line 85 (test_str_with_file(self)) r=Sylvestre

Comment 4

[Kanishk]

Attachment: Bug 1647260 - removed the method previously on line 85 (test_str_with_file(self)) r=Sylvestre,mossop

Comment 5

[Kanishk]

Attachment: Bug 1647260 - renamed one declaration of test_str_with_file to test_repr_with_file and added ".txt" extension in expected error string for assert. r=mythmon,Sylvestre

Comment 6

[Kanishk]

Hey, this is my first commit to the Mozilla codebase. I've added the author of the file as well as the reporter of the bug as reviewers.

Comment 7

[Michael Cooper [:mythmon]]

The instructions in comment 0 are not correct. We should not remove the "duplicate" function. Each function in this file is a test, and the test in question is unique. The name is a duplicate, so the name should be changed, instead of the function being removed.

Comment 8

[Kanishk]

(In reply to Michael Cooper [:mythmon] from comment #7)

The instructions in comment 0 are not correct. We should not remove the "duplicate" function. Each function in this file is a test, and the test in question is unique. The name is a duplicate, so the name should be changed, instead of the function being removed.

I understand. I will rename one of the functions. On searchfox I see that there are no references to the function test_str_with_file, so it does not need to be renamed anywhere else I assume?

Comment 9

[Michael Cooper [:mythmon]]

That's correct. This function is automatically discovered by the test runner. The name doesn't matter, except that it start with test_ and it be descriptive of the test. No where else needs updated.

Comment 10

[Kanishk]

I have renamed it properly (I think it was misnamed by mistake originally, from the body of the test, it is clear that one of them should be named test_repr_with_file). I have submitted the patch for review.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/97213363d221
renamed one declaration of test_str_with_file to test_repr_with_file. r=mythmon

Comment 12

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 97213363d221 (bug 1647260) for linux build bustages at test_gen_feature_definitions.py.
https://hg.mozilla.org/integration/autoland/rev/d0b5cdb6b19486225a1ea90a5f962cd3e567eee6

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=97213363d221449b25db3702771e606a24ba83e8&selectedTaskRun=LlbWm6mjQ2qC-d2yj9cFQw.0

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=307997864&repo=autoland&lineNumber=61226

Comment 13

[Kanishk]

(In reply to Cristian Brindusan [:cbrindusan] from comment #12)

Backed out changeset 97213363d221 (bug 1647260) for linux build bustages at test_gen_feature_definitions.py.
https://hg.mozilla.org/integration/autoland/rev/d0b5cdb6b19486225a1ea90a5f962cd3e567eee6

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=97213363d221449b25db3702771e606a24ba83e8&selectedTaskRun=LlbWm6mjQ2qC-d2yj9cFQw.0

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=307997864&repo=autoland&lineNumber=61226

This seems to be an error thrown by the test test_str_with_file, which wasn't changed by me.
From the failure logs, it looks like we are missing the .txt extension in our expected error message.

What should be done?

Comment 14

[Kanishk]

I added .txt extension in expected error string for assert in assert str(error) == 'In file "some/bad/file.txt": oops'.

The builds that were previously failing in the autoland are now passing.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdb80dc5b44097ee3ea580ac50f271cbebaa8ced

I've re-opened the patch for review.

Comment 15

[Kanishk]

The patch has been accepted but not landed yet. Anything remaining to be done wrt this bug?

Comment 16

[Sylvestre Ledru [:Sylvestre]]

See:
https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html#to-push-a-change-in-the-code-base
(i did it for you)

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/97cef5b7f906
renamed one declaration of test_str_with_file to test_repr_with_file and added ".txt" extension in expected error string for assert. r=mythmon

Comment 18

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/97cef5b7f906