Closed Bug 1087682 Opened 10 years ago Closed 10 years ago

Add a manifestparser test for the case where there is no manifest

Categories

(Testing :: Mozbase, defect)

Product:
Testing
Component:
Mozbase
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla36

People

(Reporter: jgriffin, Assigned: bitgeeky, Mentored)

References

Details

(Whiteboard: [good first bug][lang=python])

User Story

We should have a unittest for manifestparser to test the case where we create a TestManifest object with no manifests specified.

manifestparser is part of mozbase; for development information, see https://wiki.mozilla.org/Auto-tools/Projects/Mozbase.

For more information on manifestparser, see http://mozbase.readthedocs.org/en/latest/manifestparser.html

Attachments

(1 file, 1 obsolete file)

Bug-1087682-v2.patch
2.88 KB, patch
jgriffin
: review+
whimboo
: feedback+
Details | Diff | Splinter Review 
Henrik explains this case in https://bugzilla.mozilla.org/show_bug.cgi?id=1056045#c11

We regressed this with bug 1061982 and fixed it with bug 1086678, but we don't yet have a test for it.  We should add one.
User Story: (updated)
Hi, I'll like to work on this one.
Assignee: nobody → mozpankaj1994
Attached patch Bug-1087682.patch (obsolete) — — Splinter Review 
Added the required Unit test, test passes on running locally. Please let me know if it looks good.
Attachment #8515578 - Flags: review?(jgriffin)
Attachment #8515578 - Flags: review?(hskupin)
Comment on attachment 8515578 [details] [diff] [review]
Bug-1087682.patch

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

::: testing/mozbase/manifestparser/tests/test_testmanifest.py
@@ +106,5 @@
> +        """
> +        Test TestManifest without any manifest, see
> +        https://bugzilla.mozilla.org/show_bug.cgi?id=1087682
> +        """
> +        manifest = TestManifest(manifests=None, strict=False)

I wonder how the manifestparser behaves with [] as specified for manifests. If there is no test for that yet, maybe adding one more?
Attachment #8515578 - Flags: review?(hskupin) → feedback+
Comment on attachment 8515578 [details] [diff] [review]
Bug-1087682.patch

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

This looks great.  Can you please also add a test for where manifests=[], as Henrik suggests?  Thanks!
Attachment #8515578 - Flags: review?(jgriffin) → review-
Added the required test, test passes on running locally. Please review.
Attachment #8515578 - Attachment is obsolete: true
Attachment #8516644 - Flags: review?(jgriffin)
Attachment #8516644 - Flags: feedback?(hskupin)
Comment on attachment 8516644 [details] [diff] [review]
Bug-1087682-v2.patch

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

Looks fine to me. Thanks.
Attachment #8516644 - Flags: feedback?(hskupin) → feedback+
Comment on attachment 8516644 [details] [diff] [review]
Bug-1087682-v2.patch

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

lgtm, thank you!
Attachment #8516644 - Flags: review?(jgriffin) → review+
try is green
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/370927f1465a
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: