Closed Bug 1018226 Opened 10 years ago Closed 8 years ago

Replace Mochitest assertion methods with Assert.jsm equivalents

Categories

(Testing :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

(Depends on 1 open bug)

Details

(Whiteboard: p=5)

Attachments

(2 files)

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

Since Assert.jsm has landed, it'd be nice to reduce code duplication due to various flavors of assertion methods defined by Mochi.

When function names between a test harness assertion and an Assert.jsm equivalent are incompatible, both will be kept to ensure backward compatibility.
If a mass-change to Assert.jsm assertions is preferred after all, a follow-up bug would be more appropriate.
Flags: firefox-backlog+
Hi Mike, are you taking Bug 1018226 for this iteration or just adding it to the product backlog?
Flags: needinfo?(mdeboer)
(In reply to Marco Mucci [:MarcoM] from comment #1)
> Hi Mike, are you taking Bug 1018226 for this iteration or just adding it to
> the product backlog?

Backlog only, for now ;)
Flags: needinfo?(mdeboer)
Gijs, due to a faulty implementation of `isnot()`, your assertion falsely passed - `newSingleWrapper.provider` and `singleWrapper.provider` ARE in fact both 'xul'!

When I switched to Assert.jsm methods, the assertion - correctly - yielded an error.

I don't know if this reveals an error in CustomizableUI.jsm, that's why I flagged you for review.
Attachment #8431693 - Flags: review?(gijskruitbosch+bugs)
(In reply to Mike de Boer [:mikedeboer] from comment #4)
> Created attachment 8431693 [details] [diff] [review]
> Patch 2: make existing tests compatible with global Assert.jsm methods
> 
> Gijs, due to a faulty implementation of `isnot()`, your assertion falsely
> passed - `newSingleWrapper.provider` and `singleWrapper.provider` ARE in
> fact both 'xul'!
> 
> When I switched to Assert.jsm methods, the assertion - correctly - yielded
> an error.
> 
> I don't know if this reveals an error in CustomizableUI.jsm, that's why I
> flagged you for review.

Yeah, this shouldn't be happening. The new wrapper should have provider === 'api'.
(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to Mike de Boer [:mikedeboer] from comment #4)
> > Created attachment 8431693 [details] [diff] [review]
> > Patch 2: make existing tests compatible with global Assert.jsm methods
> > 
> > Gijs, due to a faulty implementation of `isnot()`, your assertion falsely
> > passed - `newSingleWrapper.provider` and `singleWrapper.provider` ARE in
> > fact both 'xul'!
> > 
> > When I switched to Assert.jsm methods, the assertion - correctly - yielded
> > an error.
> > 
> > I don't know if this reveals an error in CustomizableUI.jsm, that's why I
> > flagged you for review.
> 
> Yeah, this shouldn't be happening. The new wrapper should have provider ===
> 'api'.

Err, when I run this test this manually (./mach mochitest-browser browser/components/customizableui/browser_941083_invalidate_wrapper_cache_createWidget.js --jsdebugger) and inspect the two wrappers, that is indeed the case, and the old one has provider === 'xul'. So I don't know what you're seeing, but please check it again and/or provide more detailed STR.
Attachment #8431693 - Flags: review?(gijskruitbosch+bugs) → review-
Ah, I see I didn't provide the Try build where I saw this happening: https://tbpl.mozilla.org/?tree=Try&rev=9643b583c21e
(In reply to Mike de Boer [:mikedeboer] from comment #7)
> Ah, I see I didn't provide the Try build where I saw this happening:
> https://tbpl.mozilla.org/?tree=Try&rev=9643b583c21e

So what are the STR here? What patches do I need on vanilla fx-team? Can you reproduce this yourself? Is it Linux specific? Does it only happen if you run the entirety of mochitest-browser ?
Flags: needinfo?(mdeboer)
Comment on attachment 8431688 [details] [diff] [review]
Patch 1: make Assert.jsm methods globally available and deprecate Mochitest-browser's custom assert methods

I'm not a peer of this code, but I'm very strongly opposed to adopting a new set of assertion functions without a clear value proposition which I think is currently lacking from this patch.  So, r-.  :-)
Attachment #8431688 - Flags: review-
Also a very important point is that commonjs compatibility should be a non-goal for mochitest-*, as I doubt that the target audience of those frameworks would be very familiar/used to commonjs.
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #9)
> I'm not a peer of this code, but I'm very strongly opposed to adopting a new
> set of assertion functions without a clear value proposition which I think
> is currently lacking from this patch.  So, r-.  :-)

Part of the value proposition is Assert.jsm supports machine readable output. Also, Assert.jsm is easy to extend and improve. Contrast with the one-off nature of the harnesses today. Assert.jsm is a huge step in the direction we need to go. f+.
OK now I am confused. Why is machine-readable output tied to the names of the assertions? There is already work to add machine-readable output (i.e. structured logging) to mochitest that's independent of this work.
Assert.jsm has abstracted reporting, which makes it simple to add custom/multiple reporters to facilitate machine readability.
I still don't see why this is a major selling point for integrating this into mochitest. Do we actually care about more than a) the current in-harness reporting and b) structured logging? Given that those already exist in mochitest (at least in patch form), there would have to be some other output-related requirement, that couldn't be served by processing the structured log data, in order to make result reporting a compelling reason for going ahead with this patch.
Ignoring the reporting requirements, Assert.jsm is easier to modify, extend, and improve. It's also similar across test suites. Consistency is a good thing. Personally, I'm tired of every test flavor being a one-off and welcome our shared JSM overlords.
(In reply to Gregory Szorc [:gps] from comment #13)
> Assert.jsm has abstracted reporting, which makes it simple to add
> custom/multiple reporters to facilitate machine readability.

I don't think the user facing testing API has anything to do with how the reporting happens behind the scenes.  Can you please clarify?

(In reply to Gregory Szorc [:gps] from comment #15)
> Ignoring the reporting requirements, Assert.jsm is easier to modify, extend,
> and improve.

How so?  I've modified SimpleTest tons of times, and have had no problems doing so.

> It's also similar across test suites. Consistency is a good
> thing.

That says nothing in defense of Assert.jsm.  ;-)  But I do agree with you that consistency is good.

> Personally, I'm tired of every test flavor being a one-off and
> welcome our shared JSM overlords.

I'm not sure what problem you're trying to solve here.  Note that I'm not objecting to changing things per se, I'm objecting to changing them without a very good reason.  Changing the testing API for something as huge as mochitest-* should have an extremely high bar.  So, let's discuss why we want to chaneg the testing API please.
Depends on: 1020875
Comment on attachment 8431688 [details] [diff] [review]
Patch 1: make Assert.jsm methods globally available and deprecate Mochitest-browser's custom assert methods

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

I'm going to clear this review given the controversy over changing the existing assert methods. When we've sorted out the best course of action we can revisit.
Attachment #8431688 - Flags: review?(ted)
Flags: needinfo?(mdeboer)
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: