Closed Bug 1282910 Opened 3 years ago Closed 3 years ago

test_security-info-certificate.js uses issuerOrganizationalUnit when the property being tested is issuerOrganizationUnit

Categories

(DevTools :: Console, defect, P3)

defect

Tracking

(firefox50 affected, firefox51 fixed)

RESOLVED FIXED
Firefox 51
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: keeler, Assigned: mgsudhanva, Mentored)

Details

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

Attachments

(1 file, 2 obsolete files)

In the beginning, the interface nsIX509Cert was written with a typo: what should have been the "issuerOrganizationalUnit" field was declared the "issuerOrganizationUnit". It would be nice to fix this, but doing so risks silently breaking existing code outside of the tree (e.g. add-ons). Maybe we still will, but in the meantime there's a preexisting issue in that test_security-info-certificate.js refers to the wrong field:

  equal(result.issuer.organizationalUnit, DUMMY_CERT.issuerOrganizationalUnit,
    "Organizational unit of the issuer is correct.");

("issuerOrganizationalUnit" should be "issuerOrganizationUnit")
Component: Developer Tools → Developer Tools: Console
Priority: -- → P3
Whiteboard: [good first bug][lang=js]
Mentor: jsnajdr
hello , i would like to work on this bug .How do i begin ?
(In reply to mgsudhanva from comment #1)
> hello , i would like to work on this bug .How do i begin ?

Thanks for your interest!

The first thing you need to do is to checkout the Firefox sources from Mercurial or Git and build it. Please follow the hacking guide here:

https://wiki.mozilla.org/DevTools/Hacking

Additionally, if you'd prefer working with the Git clone of the Firefox repository on Github instead of setting up Mercurial, then this MDN page has the info on Git setup:

https://developer.mozilla.org/en-US/docs/Tools/Contributing

Then you can start hacking on the actual bug. You want to have a look at these lines:

http://searchfox.org/mozilla-central/source/devtools/shared/webconsole/test/unit/test_security-info-certificate.js#46,54

and fix the property names.
I have changed the property names , anything more i need to do ?
Attachment #8781065 - Flags: review?(jsnajdr)
Comment on attachment 8781065 [details] [diff] [review]
Proposed patch with the property names changed

Review of attachment 8781065 [details] [diff] [review]:
-----------------------------------------------------------------

The changes themselves look good, but the patch is wrongly formatted and can't be applied: it shows difference between file "original.js" and "test_security-info-certificate.js", without any path information.

After the patch is fixed, we'll run a try build to see that all tests still pass.
Attachment #8781065 - Flags: review?(jsnajdr) → review-
Assignee: nobody → mgsudhanva
Attached patch Changed the format of the patch (obsolete) — Splinter Review
Created the patch using hg-mercurial
Attachment #8782319 - Flags: review?(jsnajdr)
Attachment #8781065 - Attachment is obsolete: true
(In reply to mgsudhanva from comment #5)
> Created the patch using hg-mercurial

Thank you, this is much better and can be applied cleanly. Before we can submit a try run and land the patch, please change the commit message. It should be in format:

Bug 1282910 - Fix issuerOrganizationUnit property name in test_security-info-certificate.js r=jsnajdr

That is, it needs to contain:
- the bug number (many automation tools rely on this)
- short description of what's been changed
- the reviewer's name

Also, when posting a new version of patch as an attachment to Bugzilla, mark the previous one as "obsolete" - there is a checkbox for that in the Bugzilla UI.
Attachment #8782319 - Flags: review?(jsnajdr)
Comment on attachment 8782321 [details] [diff] [review]
test_security-info-certificate.js uses issuerOrganizationalUnit when the property being tested is issuerOrganizationUnit

Review of attachment 8782321 [details] [diff] [review]:
-----------------------------------------------------------------

The patch is now perfect, including the commit message, and all tests in the try run have successfully passed. Ready to land. Thanks for fixing this bug!
Attachment #8782321 - Flags: review?(jsnajdr) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/7db43c31bdb7
Fix issuerOrganizationUnit property name in test_security-info-certificate.js. r=jsnajdr
Keywords: checkin-needed
[good first bug] whiteboard -> keyword mass change
Keywords: good-first-bug
https://hg.mozilla.org/mozilla-central/rev/7db43c31bdb7
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.