Closed Bug 634225 Opened 13 years ago Closed 13 years ago

Implementation of an assertion module

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Product:
Mozilla QA Graveyard
Component:
Mozmill Tests
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

(Whiteboard: [module-refactor][mozmill-2.0+])

Attachments

(1 file, 8 obsolete files)

Patch v4.1
26.45 KB, patch
gmealer
: review+
Details | Diff | Splinter Review 
We need a shared module for assertions, which can be used in our tests to assert in certain situations. I will come up with a proposal of a first set of methods and how failures will be turned into error messages.
Whiteboard: [module-refactor]
I did a check through our existing tests yesterday and today, and also collected some information from other test harnesses. As what I got so far is, that the current assert methods in the Mozmill controller are mostly very specific for individual parts. In general a subset of those would be enough.

Lets take for example 'assertJSProperty'. It asserts for a property on a JS object. Fine. But there are a lot cases we would need a method like getJSProperty() on an element. It could then be use for if conditions, or loops. Right now that's not possible. Side-effect would be that the test itself could call an assert like that:

Assert.isTrue(elem.getJSProperty("disabled"), "Element is disabled");

instead of (what we have now):

controller.assertJSProperty(elem, "disabled");

The same applies to a couple of other assert methods currently attached to the contoller object.

Given our shared module refactoring we want to have a separate assert module, which contains methods completely decoupled from the controller. At the moment I have the following assert methods in mind:

* assert.isTrue(value, message)

Here we can cover mostly all assertions with one operand involved. Means we can check for valid booleans, strings, numbers, and objects (null/undefined). You can also call it with isTrue(!value, message) to test for false. I don't think we explicitly need a isFalse() method. But if wanted we can add it.

* assert.isEqual (cond1, cond2, message)

Can be used to compare two operands. It will use the triple operator to also check its type.

* assert.isNotEqual (cond1, cond2, message)

Counter equivalent to isEqual, but for the negative case.

* assert.contains (string, substring, message)

Checks if the string contains the given substring.

* assert.matches (string, regexp, message)

Enhanced version of contains which uses a regex for the check. 

Not sure yet, if we need both contains and matches methods. Personally I would prefer the matches method, which is also part of the Jetpack assert module.

In general I don't see any instance where we would have to call an assert method with a callback. It makes it a lot easier to use now and also cleans up a lot of extra code in our tests.

The above methods I would propose for the first iteration of our tests. Has anyone complains about it? Have I forgotten something?
Oh and something I forgot to mention, all those assert will be non-fatal! That means the test will not be aborted when the assert fails. We simply log a failure to the events module. That's one of the points we currently miss in the Mozmill's controller object.
These look good. We do need some way to fatally assert, I think (it does come up, though it's the exceptional case, not the usual). So, I think it'd be best if the functions also return true/false based on pass/fail respectively, and that we provide an abort() function.

Then we get:

if !assert.isTrue(foo, "message")
  abort();

Also, I find it useful to have just assert.Pass(message) and assert.Fail(message). That gives you a direct way to post results from a more complicated control structure without using dummy variables and such. They're super-easy to write, so I'd appreciate having them in there too.
(In reply to comment #3)
> if !assert.isTrue(foo, "message")
>   abort();

Wouldn't be the best way, because we lose the built-up message. We could add an optional last parameter which would control that.

> Also, I find it useful to have just assert.Pass(message) and
> assert.Fail(message). That gives you a direct way to post results from a more

Dang. Missed that. Those two functions are already in-place. I simply forgot to mention them in my last comments.
(In reply to comment #1)
> The above methods I would propose for the first iteration of our tests. Has
> anyone complains about it? Have I forgotten something?

This is so close to the CommonJS API that I think we might as well follow a standard, the diff is:

CommonJS:
assert.isTrue() -> assert.ok()
assert.isEqual() -> assert.equal()
assert.isNotEqual() -> assert.notEqual()

+ assert.deepEqual()  - for comparing objects and arrays
+ assert.notDeepEqual()
+ assert.strictEqual()  - for comparing with ===

- assert.matches - but we can always add this.
(In reply to comment #3)
> Also, I find it useful to have just assert.Pass(message) and
> assert.Fail(message). That gives you a direct way to post results from a more
> complicated control structure without using dummy variables and such. They're
> super-easy to write, so I'd appreciate having them in there too.

this sounds like an abuse of the assert module? maybe there should be some kind of a logging module?
(In reply to comment #5)
> This is so close to the CommonJS API that I think we might as well follow a
> standard, the diff is:

I would be in favor of using the same path as what is planned for Mozmill 2.0. That would allow us to easily switch the modules later.

> CommonJS:
> assert.isTrue() -> assert.ok()
> assert.isEqual() -> assert.equal()
> assert.isNotEqual() -> assert.notEqual()

I don't have any problem with it. Sounds fine.

> + assert.deepEqual()  - for comparing objects and arrays

That's a quite complex function. If it will be able to cover everything, especially when you have a lot if inheritance in use.

> + assert.notDeepEqual()

Isn't that equal()?

> + assert.strictEqual()  - for comparing with ===

As we have decided on we always want to use triple operators for comparisons. Doing not so can cause a lot of unforeseen behavior. Crockford also explains it very well in his book.

> - assert.matches - but we can always add this.

Very useful for us. Something like that is used in a couple of tests and existing modules.
(In reply to comment #6)
> (In reply to comment #3)
> > Also, I find it useful to have just assert.Pass(message) and
> > assert.Fail(message). That gives you a direct way to post results from a more
> > complicated control structure without using dummy variables and such. They're
> > super-easy to write, so I'd appreciate having them in there too.
> 
> this sounds like an abuse of the assert module? maybe there should be some kind
> of a logging module?

Right now, I'm using those methods from all the assert methods. Those handle the creation of the failure message. I simply don't export those. Whatever we will decide on, it's a simple change to let those be used from tests.
I have found an interesting link re fatal vs. non-fatal methods. Google is using assert for fatal and expect for non-fatal checks. Given that it would leave it up to the test itself, which method has to be used.

http://code.google.com/p/googletest/wiki/Primer#Assertions
A quick heads-up on the output. In case of equal the following is reported:

{"assertion": {"function": "testElements", "message": "true is equal to false - got 'true', expected 'false'", "fileName": "resource://mozmill/modules/frame.js -> file:///Volumes/data/testing/mozmill/api-refactor/firefox/testLocationBar.js", "lineNumber": 48}}

I was able to strip down the stack to the test which is reporting the failure and not the module which contains the assert method. The only left-over part here is, that I have to strip the secmod prefix from the fileName. Than it is totally easy to identify the failing check.
(In reply to comment #6)
> (In reply to comment #3)
> > Also, I find it useful to have just assert.Pass(message) and
> > assert.Fail(message). That gives you a direct way to post results from a more
> > complicated control structure without using dummy variables and such. They're
> > super-easy to write, so I'd appreciate having them in there too.
> 
> this sounds like an abuse of the assert module? maybe there should be some kind
> of a logging module?

Re: common.js, I don't have much opinion here. We can follow their standard, perl Test::More standard (which is very close), whatever. As long as we cover the basic functionality, I'm good.

But why do you think this is abuse? There are test situations where you end up unavoidably doing a complex verification in something obnoxious like an if/else-if tree or a switch-type statement. Without explicit pass/fail, you have to save off the condition in a dummy variable and then do an ok/isTrue on it later. The problem is that the logic is separated from the assertion, and it's harder to read. Pass/Fail is much more explicit. Again, this is exceptional, but very useful when you need it.

Henrik: why would we lose the message in the if !foo abort() situation? The message would have come from the failure logging assertion; it would be immediately followed by the test abort, marking the above assertion as fatal.
Oh, but I will say, yes I'd also like to be able to log comments into the results that aren't associated with a pass/fail result, but just annotate the flow of the test.

That's not what I'm talking about here, though. I'm talking about being able to craft your own domain-dependent assertion control structure that's more complex than a simple ok() or deepCompare() without having to resort to workaround crap like ok(true, "Message").
(In reply to comment #5)
> This is so close to the CommonJS API that I think we might as well follow a
> standard, the diff is:

Heather, do you have a link to a current implementation of such an assert module? I can find a lot of specs in the web, but no real code.
(In reply to comment #13)
> (In reply to comment #5)
> > This is so close to the CommonJS API that I think we might as well follow a
> > standard, the diff is:
> 
> Heather, do you have a link to a current implementation of such an assert
> module? I can find a lot of specs in the web, but no real code.

The most used implementation is the node.js one which was taken from narwhal: https://github.com/ry/node/blob/master/lib/assert.js
(In reply to comment #12)
> Oh, but I will say, yes I'd also like to be able to log comments into the
> results that aren't associated with a pass/fail result, but just annotate the
> flow of the test.
> 
> That's not what I'm talking about here, though. I'm talking about being able to
> craft your own domain-dependent assertion control structure that's more complex
> than a simple ok() or deepCompare() without having to resort to workaround crap
> like ok(true, "Message").

Ah, good point, jetpack seems to also have that https://jetpack.mozillalabs.com/sdk/1.0b2/docs/#module/api-utils/unit-test.
Attached patch WIP v1 (obsolete) — — Splinter Review 
First WIP for that parts we have agreed on. I have checked the other frameworks and those are kinda special in their implementation with a lot of dependencies. So not sure how easily those could be ported. But if requests will show up we should be able to include those too.

This code only handles the non-fatal way of assertions. For the fatal ones we first need an agreement how we want to further proceed. Do we want to have separate methods like in the Google framework, another parameter for the functions (which I think is not a good idea), or should the test throw an exception if a fatal assertion is wanted? Either way would require some refactoring of the WIP.

Asking for feedback from heather so that we are going into the right direction and can support the a-team for Mozmill 2.0.
Attachment #513289 - Flags: feedback?(fayearthur+bugs)
My preferred solution: separate functions a la Google. 

Easy solution: return bools and provide an abort() exception-throwing routine.

Possible route: start with easy solution, add separate functions later.
If we would go with separate functions should we put those into another module, i.e. expect.js which can than be used like:

expect.ok(true, "true is true");

I would really like to get feedback from Heather and Clint especially for Mozmill 2.
Attachment #513289 - Flags: feedback?(gmealer)
Hey folks,

A little behind as usual in the old bugmail.

So, I like the idea of 'expect' for non-fatal failure checks and 'assert' for fatal ones.  This way we can have one assertion library that will handle both rather than the schizophrenic approach that mozmill 1.5.x uses.

* The methods from expect and assertion must be the same, and take the same parameters.  i.e. if there is an expect.foo(a, b) then there should be a matching assert.foo(a, b)
* We don't want to put a ton of special case methods on these interfaces.  They need to be as clean as we can make them.  I don't follow Geo's assertion that we need a assert.Fail() and an assert.Pass().  Can you write some example code to show why these types of APIs are useful?  I keep thinking you mean:
if !doSomething() {
  assert.Fail(); // we totally shouldn't have returned 0 there!
}
Which I would prefer to specify as:
assert.ok(doSomething(), "doSomething should never fail");

I don't see any purpose in the .contains API.  That looks like bloat.  I think the matches one is very useful and is a super set of the .contains api, so I vote we keep matches.

Also, I like using the commonJS names.  Everywhere possible we should try to move toward using more or less standard names for these types of things.  The reason I particularly like the commonJS names is that it maps a little more closely with how mochitest assertions work, which is important so that we keep the barrier low for test writers.
Whiteboard: [module-refactor] → [module-refactor][mozmill-2.0+]
Comment on attachment 513289 [details] [diff] [review]
WIP v1

looks good. I do think the contains method is a bit redundant. It could depend on how much use you'd get out of it.

I'm also for having two separate modules for fatal and non-fatal assertions, hopefully sharing some code.
Attachment #513289 - Flags: feedback?(fayearthur+bugs) → feedback+
(In reply to comment #20)
 out of it.
> 
> I'm also for having two separate modules for fatal and non-fatal assertions,
> hopefully sharing some code.
Hopefully sharing 98% of their code :)
Clint, hard to come up with a toy example off the top of my head, re: pass/fail. I've generalized it already: you use it when a simple comparison isn't what you need to say "if the code got here, you passed, and if it got here, you failed."

Frankly, I was kind of hoping people would take my word that I've been in situations where it's useful, and also that its presence in other frameworks (Google, test::more, nearly every other framework I've encountered) is evidence of its usefulness.

Here's what test::more says about their pass/fail:

"Sometimes you just want to say that the tests have passed. Usually the case is you've got some complicated condition that is difficult to wedge into an ok(). In this case, you can simply use pass() (to declare the test ok) or fail (for not ok). They are synonyms for ok(1) and ok(0)."

It then goes on to say "use very, very sparingly", which I agree with. Also, your example is broken because you can never just have a pass or fail, it has to be an either/or or else you have a test result that never appears for one condition.

The point is to not assume the assertion module knows best in all cases, and to allow client code to do exactly what it needs in the outside instances it needs to.

Re: contains, I see your point but disagree with you. The point is to get assert/expect routines that are specific to their exact purpose. 

Technically, all you need is ok/not_ok (or really, pass/fail). All the others are more specific so they can have better output messages and make the code read more specifically. That means they absolutely should overlap. It's exactly like a wrapper function around a one-liner that's there just to give it a more meaningful name. That's not bloat, it's making the client code more maintainable.

I fully agree re: expect vs. assert needing total parity. And I like the idea of two separate modules, but wonder if having to always require them will be a pain. Maybe our proposed init functions that automatically require certain modules will mitigate that.
(In reply to comment #21)
> (In reply to comment #20)
>  out of it.
> > 
> > I'm also for having two separate modules for fatal and non-fatal assertions,
> > hopefully sharing some code.
> Hopefully sharing 98% of their code :)

No, hopefully with assert versions wrapping expect versions in all cases.

assert.foo() { if !expect.foo() throw ... }
(In reply to comment #23)
> (In reply to comment #21)
> > (In reply to comment #20)
> >  out of it.
> > > 
> > > I'm also for having two separate modules for fatal and non-fatal assertions,
> > > hopefully sharing some code.
> > Hopefully sharing 98% of their code :)
> 
> No, hopefully with assert versions wrapping expect versions in all cases.
> 
> assert.foo() { if !expect.foo() throw ... }

Indeed.  Thanks for the explanation of the pass/fail thing.  I've been using ok(true)/ok(false) for too long I guess.  We can add it now that I see where you're going with it.
Attached patch WIP v1.1 (obsolete) — — Splinter Review 
Next iteration of the patch. It's still a WIP and needs some tweaks. I will come up with a full description later today.
Attachment #513289 - Attachment is obsolete: true
Attachment #513289 - Flags: feedback?(gmealer)
Attached patch WIP v1.2 (obsolete) — — Splinter Review 
Now the full patch.
Attachment #514189 - Attachment is obsolete: true
Attached patch Patch v1 (obsolete) — — Splinter Review 
This patch is the first real working solution for assert and expect. It doesn't contain any tests yet, it's something I have to do later on once we have agreed on the implementation.

The patch introduces a couple of new files:
* errors.js (module for all supported Error classes)
* logging.js (module for all supported logging classes)
* assert.js (module for fatal assertions)
* expect.js (module for non-fatal assertions)

In general I have implemented a basic TestResultLogger, which is responsible for logging passes and failures in a non-fatal and fatal way. The fatal assertion is used once you specify an Error class as parameter of the TestResultLogger constructor. The logger class contains all the supported methods for assertions we have agreed on so far. I have fixed the regex issue with a workaround and removed the contains/notContains methods in favor of match/notMatch.

For the assert and the expect module we simply subclass the TestRunLogger. For asserts we specify the AssertionError as Error handler, while for expects its undefined and causes non-fatal failures.

We will have to find a good folder structure, where to place those modules. We should avoid to pollute the root of the modules subfolder. In the case above I would tend to say:

modules/errors.js
modules/logging.js
modules/test/assert.js
modules/test/expect.js

The above implementation also gives others the possibility to subclass the classes for their own implementation with enhancements, i.e. for add-ons tests.
Attachment #514190 - Attachment is obsolete: true
Attachment #514213 - Flags: feedback?(gmealer)
Attachment #514213 - Flags: feedback?(fayearthur+bugs)
Attachment #514213 - Flags: feedback?(ctalbert)
Attached patch Patch v1.1 (obsolete) — — Splinter Review 
Now for real with the non-negated patch.
Attachment #514213 - Attachment is obsolete: true
Attachment #514217 - Flags: review?(gmealer)
Attachment #514217 - Flags: feedback?(fayearthur+bugs)
Attachment #514213 - Flags: feedback?(gmealer)
Attachment #514213 - Flags: feedback?(fayearthur+bugs)
Attachment #514213 - Flags: feedback?(ctalbert)
Comment on attachment 514217 [details] [diff] [review]
Patch v1.1

Also requesting feedback from Clint and Heather to make sure we are on the right track.
Attachment #514217 - Flags: feedback?(ctalbert)
Comment on attachment 514217 [details] [diff] [review]
Patch v1.1

Looks basically OK, though you're doing this considerably more OOP than I would have chosen to, and possibly making it more complex by doing so. 

I'd probably have just gone for a "expects" module with funcs that post pass/fail and returned true/false, plus an asserts module that just wrapped those with a final error throw (the equiv of your if _errorObject throw logic)

I think the whole entire reason you -didn't- do it that way was so you could avoid posting a fail and then throwing the assertion, possibly causing mozmill to not throw an extra fail. 

But you're working around what I consider to be iffy architecture in Mozmill, which is that non-AssertionErrors and AssertionErrors should have been treated differently in the top-level error handler to begin with, with non-AssertionErrors causing a line-item of "ERROR" instead of "FAIL" (though this would certainly also cause a test to fail overall) and AssertionErrors causing a line-item of ABORT, separate from the fail that would have appeared just above it.

Which is to say, I'd have made even assertions post fail(), then throw their error, rather than what you're doing where assertions don't explicitly post a failure.

If Mozmill establishes a separate error class for assertions (and that they didn't was a clear mistake--IMO you never use default system errors for domain-specific things) then this behavior would be easy to add into their error handler. 

So I'm r+'ing you in that the code is fine, but still think this deserves more discussion. Especially with your concerns about waiting traits, mixins, whatnot, I honestly think this subsystem might be considerably more complex than it needs to be. I want to clearly lay out and agree on the expected behavior before you code any further.
Attachment #514217 - Flags: review?(gmealer) → review+
(In reply to comment #30)

Some additions to what we talked about in our 1-1...

> I'd probably have just gone for a "expects" module with funcs that post
> pass/fail and returned true/false, plus an asserts module that just wrapped
> those with a final error throw (the equiv of your if _errorObject throw logic)

The error object we would throw in such a case, would be disconnected from the real test done in the expect module. Even you know that the last assert could have been fired because of an expect failure, it's not always the case. So we will end up with two failures you cannot associate  easily. Also a test path like looks a bit strange:

if (!expect.equal(1, 0, "Has to fail"))
  assert.fail("We failed yeah!");

From the test writer perspective something like the following looks a way cleaner:

assert.equal(1, 0, "Has to fail"));

Beside that, you are right with the _errorClass used as parameter for the constructure of the logger class. It would need to be refactored. But as agreed on I will wait until we got feedback from Heather and Clint.

> But you're working around what I consider to be iffy architecture in Mozmill,
> which is that non-AssertionErrors and AssertionErrors should have been treated
> differently in the top-level error handler to begin with, with
> non-AssertionErrors causing a line-item of "ERROR" instead of "FAIL" (though
> this would certainly also cause a test to fail overall) and AssertionErrors
> causing a line-item of ABORT, separate from the fail that would have appeared
> just above it.

I haven't taken care of how Mozmill report those different kind of failures. It's out of scope for those modules. 

> Which is to say, I'd have made even assertions post fail(), then throw their
> error, rather than what you're doing where assertions don't explicitly post a
> failure.

It's something we should take about in Mozmill 2, but for now I think it will cause confusion when we report the same failure multiple times. IMO it's worth filing a bug for Mozmill.
Comment on attachment 514217 [details] [diff] [review]
Patch v1.1

>diff --git a/firefox/testLocationBar.js b/firefox/testLocationBar.js
>diff --git a/modules/assert.js b/modules/assert.js
>index 0d9b847..658aed6 100644
>--- a/modules/assert.js
>+++ b/modules/assert.js
>@@ -14,11 +14,11 @@
>  * The Original Code is MozMill Test code.
>  *
>  * The Initial Developer of the Original Code is the Mozilla Foundation.
>- * Portions created by the Initial Developer are Copyright (C) 2010
>+ * Portions created by the Initial Developer are Copyright (C) 2011
>  * the Initial Developer. All Rights Reserved.
>  *
>  * Contributor(s):
>- *   Geo Mealer <gmealer@mozilla.com>
>+ *   Henrik Skupin <hskupin@mozilla.com>
First, on simple good governance practices, we don't ever do this without some kind of decent justification.  The idea here is that person should be honored as a contributor even if the code has been changed, they still worked on some version of it, and they deserve credit.  Although I'm sure this was just an oversight, this is usually seen as aggressive behavior.  Since we all know each other it's not so much of an issue but if Geo were an outside volunteer contributor, I'd be yelling at you instead of calmly reminding you about this.

As for the rest of the patch, I'm really confused.

I understood that we were changing the core assert module inside of mozmill, and this is all changes to shared modules in mozmill-tests.  Furthermore, the fixes and the abstraction of testlogger should also be moved into mozmill, and I think that we want to formalize that so that we can easily and properly report errors. We do NOT want shared modules reporting errors one way, and the entire system reporting errors some other way, so this all needs to be moved into the core harness.

Also, I thought we were going to take the implementation of the assert module from CommonJS and append to it what we needed rather than craft our own.  Is there a reason you chose not to do that?

Also, I want one assert module that provides both expect and assert functionality.

Tests should only need to import one "assert.js" and then they can use either expects or asserts etc.  I don't think the test should import TestLogger than then magically get assertions that come along for the ride, that's requiring too much knowledge on the test writer's part, and I'd prefer these sorts of APIs to be as explicit as we can make them.

I do like the way you did the inheritance so that future tests can subclass these things, but again, we really need this in the core of mozmill, and our goal should be to make this extensible yes, but also to provide an API that doesn't need extending for 99.99999% of cases.  The Assert layer/the testLogging layer should be core to Mozmill, and should really not be something that tests are needing to subclass/extend.  If tests need to regularly extend/modify these core classes, then our API is a failure. (And yes, I'm in total agreement that the 1.5.x API is a failure).  

I like the gist of what you're doing, but we need this in Core mozmill.  We need to wire in strong test logging, strong assert methods into the mozmill base itself.  We need to fix bugs in that existing logic rather than working around them.  Anything less is making the mozmill app substandard and bound to fail.
Attachment #514217 - Flags: feedback?(ctalbert) → feedback-
Attached patch Patch v2 (obsolete) — — Splinter Review 
Thanks Clint for the feedback and the last patch and the nearly 1h phone call we had for discussing that. Right before our call I already have made changes for a better class structure. The one from the last patch was really broken. As attached you will see the updated patch we have agreed on in relationship to Mozmill. I would Clint let comment on that first but would like to also get feedback from Heather and Geo again.

Re the list of contributors and why I haven't listed Geo. The modules have been completely rewritten. None of the code we had before is existent anymore. But if that would mean we still have to carry over existent contributors I will do that in one of the follow-up patches.
Attachment #514217 - Attachment is obsolete: true
Attachment #514549 - Flags: review?(gmealer)
Attachment #514217 - Flags: feedback?(fayearthur+bugs)
Attachment #514549 - Flags: feedback?(ctalbert)
Attachment #514549 - Flags: feedback?(fayearthur+bugs)
Comment on attachment 514549 [details] [diff] [review]
Patch v2

So we talked on the phone about this.  The QA team need this for their shared module refactor, which is why it is done the way it is done.

Henrik and I talked for a while about the implementation and I like the implementation, liked the idea in the other patch.  The munging of the stackframe to get around mozmill bugs still makes me queasy but there isn't much of a way to do that well until we fix it in 2.0.

So, let's talk about taking this for 2.0.
* I propose we take the patch wholesale - consume all of the stuff with the following caveats:
** We remove the google class inheritance thingy for something like trait.js (what the jetpack folks are using) or something else.
** We remove the stack frame stuff and investigate doing some frame caching in mozmill so that when a test throws an assert, we report the line that threw the assert as the top line of the failing stack.  This will also make failures easier to spot.  We should optionally turn off frame caching when the --debug parameter is set.
** We should ensure that when errors are thrown in shared modules it looks the same as an assertion thrown from a test.  (i.e. shared modules should no longer have assertions/expects in 2.0-land.  Asserts expects will be reserved for tests and if a shared module finds something is incorrect, then it should throw).  This is a kind of hand-wavy thing that Henrik and I were discussing, but I think it makes sense to have shared modules behave like part of the core mozmill and less like tests.
** We should take the interfaces and implementation for these directly into mozmill 2.0, and place the assert.logging and expect.logging into assert and expect APIs on the global object, just like Henrik does in his init script.  We'll do that in frame.js.
** If we find that tests require these classes be extended on a regular basis (we'll be able to discover this as Henrik and Geo complete their shared module refactor) then we should add/modify the interface in TestLogger so that we do not need to extend this class.  We should shoot for an explicit interface here that works for 99.99999% of cases.

PS I still think we should give Geo credit in the contributors line ;)
Attachment #514549 - Flags: feedback?(ctalbert) → feedback+
re: contrib, I don't really care. It was a complete rewrite. I think there may have been one of these with Henrik's name at the top and I stomped on that. At this point, I really don't know, and it's not a big deal to me.

Re: errors in the shared modules, shared modules should never, ever assert--only tests should. 

Shared modules can throw exceptions on endstate checks (e.g., I just loaded a URL, is the URL really loaded? If not, throw) but I want that distinct from an assertion that the test explicitly asked for. The reason is that those types of failures usually means a script problem that happens in setup/teardown, not an AUT problem happening in a test, and making them look different helps greatly in diagnostics.

So, in that specific instance, a loaded URL -not- loading, I'd expect the test that actually -tests- loading URLs to have an assertion FAIL, and any tests that relied on URL loads for their setup to have an ERROR that aborted them.

If this isn't clear, let's talk more. It's pretty key to how I'd like to see the framework organized.
Just re-read your comment, and I think it was me who misunderstood you. I think you said the same thing I did, but I got caught by something that said "errors should look like assertions" or similar.

Totally agree that shared-module errors should look like something that came from mozmill, vs. an assertion, but that both should be similar in the final results in that they stop a test.
My thoughts on this are:

* logging.js seems like a bit of a misnomer when there are mainly assertion functions in it.

* I missed this and can't seem to glean it from the previous comments, can you explain why not just have one object like AssertionLogger that defines .ok() and .equal() that uses the _logFail() function, then have ExpectationLogger inherit from it in the usual prototype way, just overriding the _logFail() function? Then doing:

module.exports = new AssertionLogger()
(In reply to comment #37)
> * I missed this and can't seem to glean it from the previous comments, can you
> explain why not just have one object like AssertionLogger that defines .ok()
> and .equal() that uses the _logFail() function, then have ExpectationLogger
> inherit from it in the usual prototype way, just overriding the _logFail()
> function? Then doing:

Huh, that's mostly right but the other way around. With my last update to the TestResultLogger class has been made mostly obsolete. There is nothing the ExpectLogger class adds to it. So we could really move all methods over to the ExpectLogger class. That way we do not have to introduce another 3rd module.
Attached patch Patch v3 (obsolete) — — Splinter Review 
Updated patch with a finally single assertions.js module. There was no reason to keep the logging module around as said earlier. Thanks Heather!

This patch also contains tests for the assertion classes and with those I have even found two more failures in my code. Yay for tests!

If we can agree on the code, the next step will be to create inline docs.
Attachment #514549 - Attachment is obsolete: true
Attachment #514848 - Flags: feedback?(fayearthur+bugs)
Attachment #514549 - Flags: review?(gmealer)
Attachment #514549 - Flags: feedback?(fayearthur+bugs)
Attachment #514848 - Flags: feedback?(gmealer)
Comment on attachment 514848 [details] [diff] [review]
Patch v3

Looks good. I like the new class structure better.

One thing I'm still a little unclear on (probably is fine, just want clarification): 

How come in expect it logs a fail into the frame, but in assert it only throws the error but doesn't call the parent first to log a fail?
Attachment #514848 - Flags: feedback?(gmealer) → feedback+
(In reply to comment #40)
> How come in expect it logs a fail into the frame, but in assert it only throws
> the error but doesn't call the parent first to log a fail?

Because in such a case we would report the error twice. Once for adding the failure frame and again for the assert. Once the Mozmill frame handling has been fixed including for exceptions, we can and have to change that. But in the current state we really should not report it twice.
Comment on attachment 514848 [details] [diff] [review]
Patch v3

This looks good, one note:

>+function AssertError(options) {
>+  let error = Object.create(AssertError.prototype);
>+
>+  error.message = options.message;
>+  error.fileName = options.fileName;
>+  error.lineNumber = options.lineNumber;
>+  error.function = options.function;
>+
>+  return error;
>+}

This is AssertionError in the AssertLogger(), I like AssertionError better personally, AssertError could read like "assert error".
Attachment #514848 - Flags: feedback?(fayearthur+bugs) → feedback+
(In reply to comment #42)
> This is AssertionError in the AssertLogger(), I like AssertionError better
> personally, AssertError could read like "assert error".

I have renamed AssertionError to AssertError because we now have an assertion module and I wanted to stop confusion to which class (expect, or assert) this error belongs to.
(In reply to comment #43)
> (In reply to comment #42)
> > This is AssertionError in the AssertLogger(), I like AssertionError better
> > personally, AssertError could read like "assert error".
> 
> I have renamed AssertionError to AssertError because we now have an assertion
> module and I wanted to stop confusion to which class (expect, or assert) this
> error belongs to.

I don't see any confusion either way. Asserts cause Assertions, this is well known. 

I liked the name better as AssertionError myself. FYI, Java and Python both use AssertionError, so it's least surprising, too.
AssertionError is also used by Node.js and other CommonJS conform implementations. So I have renamed AssertError back to AssertionError.
Attached patch Patch v4 (obsolete) — — Splinter Review 
With this patch I have added documentation for all new classes and instances. I have also improved the docs for the init module to expose the augmented properties in the test module. Further I have removed the "Logger" part of the class names for Expect and Assert, which shouldn't be called out.

To generate the docs we should use the outline template, because the default jsdoc template is kinda limited. Here the command which has to be run:

bash jsrun.sh -r -t=templates/outline/ --exclude="test_*" ../api-refactor/modules/
Attachment #514848 - Attachment is obsolete: true
Attachment #515454 - Flags: review?(gmealer)
Attached patch Patch v4.1 — — Splinter Review 
Forgot to pull master before creating the diff. Some of your sprint1 changes were included. Also renamed the forgotten SoftExceptLogger class to SoftExpect.
Attachment #515454 - Attachment is obsolete: true
Attachment #515455 - Flags: review?(gmealer)
Attachment #515454 - Flags: review?(gmealer)
Blocks: 634229
Henrik,

First, overall comments on docs. I'll follow this up with a diff review like you'd expect.

Thanks, btw, for the jsrun syntax. I wrestled with it briefly, didn't figure it out, so went straight to jar. You figuring it out was helpful.

I'm not sure I prefer the outline template, so that's not at all a done deal. I took a quick look at JSProton Friday, but certainly not to the point of saying "yes we're going with that," and didn't mean for you to take it as that.

If every class repeats every inherited member underneath it in the left-hand outline, big class trees like Element's start getting awfully long.

Also, this completely fails to list inheritance on the right side detail pane. The left side pretends that any given inherited member is echoed on the child class, but clicking on it takes you to the parent (even though you'd expect it to take you to the child since you don't know it's inherited). That's just broken, and unacceptable in our template.

In contrast, your inheritance (and mine) came through fine on the basic template. I don't know what you were looking for, but it was there.

This was in the Assert detail, right under the Class Summary:

Methods borrowed from class assertions.Expect:
    equal, fail, match, notEqual, notMatch, ok, pass 

Each was a link up to Expect's version. Also, the inheritance from assertions.Expect was listed at the top above the Class Summary.

It's possible we can make a hybrid version (basically "fix" Outline's problems) but that's a distraction. For now, please let's just make it look right in the default so we have a functional baseline to work from. 

We'll tackle alternate templates later once we know how it's -supposed- to work. If we'd started with Outline I'd be beating my head against the walls trying to get inheritance features to show up.
Comment on attachment 515455 [details] [diff] [review]
Patch v4.1

General doc tagging issues:

You need a @class "some description" on each initialize JSDoc so that the class index will show a summary description.

I think aStartFrame is optional with a default? If so, the syntax in the tag is:

@param {object} [aStartFrame=Components.stack]

I tend to think some of the docs from _findCallerFrame should be in the constructor as well, because it's not at all plain from the docs what "Frame of the stack to start from for the logging" means. You probably need to explain the mechanism a little in the class docs itself.

(btw, we should talk re: the "aFoo" param names so we standardize)

Re: your module namespace, if you don't make something a @memberOf anything, it goes into a section called Global: i.e. available w/o a module designator. 
Don't you think that'd be a better place for things like assert/expect than a fake namespace that doesn't exit?

Not an issue: 

I'm mildly shocked you don't need @memberOf type stuff, as well as @function/@field designators. We'll have to revisit this on widgets.js for the refactoring round. I swear that if I didn't have @memberOf on both the class (@memberOf widgets) and the members (@memberOf widgets.Element), it choked and didn't list a lot of stuff. There must be some difference, because your docs do generate fine and look just like mine. Maybe it was the "prototype" part of your @lends.

Code:

Code looks fine. Thanks for all the iterations on the design. I like where we landed.

I'm going to go ahead and r+ this. If you want to push any @class tags or doc fixes to the sprint2-assert branch, I'll merge first thing in the morning. Otherwise, we can take care of them during the refactor round.
Attachment #515455 - Flags: review?(gmealer) → review+
(In reply to comment #49)
> You need a @class "some description" on each initialize JSDoc so that the class
> index will show a summary description.

Good call. Finally missed that.

> @param {object} [aStartFrame=Components.stack]

Nice hint. It's something I haven't found yet. Looks good.

> I tend to think some of the docs from _findCallerFrame should be in the
> constructor as well, because it's not at all plain from the docs what "Frame of
> the stack to start from for the logging" means. You probably need to explain
> the mechanism a little in the class docs itself.

done.

> (btw, we should talk re: the "aFoo" param names so we standardize)

See outstanding items:
https://wiki.mozilla.org/QA/Mozmill_Test_Automation/Test_Modules_Refactor#Outstanding_Items

> I'm mildly shocked you don't need @memberOf type stuff, as well as
> @function/@field designators. We'll have to revisit this on widgets.js for the
> refactoring round. I swear that if I didn't have @memberOf on both the class
> (@memberOf widgets) and the members (@memberOf widgets.Element), it choked and
> didn't list a lot of stuff. There must be some difference, because your docs do
> generate fine and look just like mine. Maybe it was the "prototype" part of
> your @lends.

Yes, that's the reason for. Only when you lend it to the prototype of a class it will become a member and not a static element. See the last example on that page: http://code.google.com/p/jsdoc-toolkit/wiki/TagLends

> I'm going to go ahead and r+ this. If you want to push any @class tags or doc
> fixes to the sprint2-assert branch, I'll merge first thing in the morning.
> Otherwise, we can take care of them during the refactor round.

Everything has been updated on the branch. And I will merge it with the master immediately, so I can continue with the other modules.
Landed as:
https://github.com/geoelectric/mozmill-api-refactor/commit/103080145357aebca90e15f5be181416f3f3303d
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Blocks: 634227
Awesome, thanks! I'll look forward to refactoring my widget doc tags into something more reasonable.
Blocks: 635237
Blocks: 637880
I think AssertionError should be defined in the assertions module instead of a separate "error" module. It makes importing it into main Mozmill a bit easier and the only thing using AssertionError is the assertions module.
>+const { AssertionError } = require('errors');
>+


we should probably use "./errors" in the future to be consistent. It lets you leave off the "./" but it shouldn't.
Heather, can we keep this discussion ongoing on bug 637880? This one has been fixed a while back.
Blocks: 668810
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: