639691 - setup* and teardown* should not be counted as tests

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Jeff Hammel]

Currently for results reporting and all other purposes, we count
setupModule, setupTest, teardownModule and teardownTest as tests.
setupModule and teardownModule shouldn't be counted as tests at all.
the setupTest and teardownTest function should be counted as part of the test
that is being run. 

To illustrate, currently a file with setupModule and teardownModule
and setupTest and teardownTest and testFoo is counted, for all
purposes, as 5 tests.  Instead, it should be counted as one

Comment 1

[cmtalbert]

Attachment: WIP

So, this is the part of the patch that drops all passes in setup/teardown and doesn't report them.  Next, some changes must be made on the python side so that if a failure occurs in setup/teardown we fail the test.  I'm not sure how plausible that is for 2.0 though, so at this point, I'm letting fails and skips in setup/teardown be reported like normal tests in this WIP.

Comment 2

[cmtalbert]

Comment on attachment 528406 [details] [diff] [review]
WIP

Well, since this is probably what we want short term, let's see if we can take this for 2.0.  And we might punt the python side off to 2.1

Comment 3

[Jeff Hammel]

Comment on attachment 528406 [details] [diff] [review]
WIP

Review of attachment 528406 [details] [diff] [review]:

r+

::: mozmill/mozmill/extension/resource/modules/frame.js
@@ +239,5 @@
+      test.__name__ == 'teardownTest' ||
+      test.__name__ == 'teardownModule')) {
+    shouldSkipReporting = true;
+  }
+  

Is there any reason you set a variable other than just put the logic (events.fireEvent('endTest')) in here directly? Just seems funny.  Also, checking for test.__passes__ will result in the event being fired if the setup or teardown fails.  This will give inconsistent numbers, though this is already a problem we have (since if setupModule fails we don't run tests at all).

Comment 4

[cmtalbert]

(In reply to comment #3)

> ::: mozmill/mozmill/extension/resource/modules/frame.js
> @@ +239,5 @@
> +      test.__name__ == 'teardownTest' ||
> +      test.__name__ == 'teardownModule')) {
> +    shouldSkipReporting = true;
> +  }
> +  
> 
> Is there any reason you set a variable other than just put the logic
> (events.fireEvent('endTest')) in here directly? Just seems funny.  Also,
> checking for test.__passes__ will result in the event being fired if the
> setup or teardown fails.  This will give inconsistent numbers, though this
> is already a problem we have (since if setupModule fails we don't run tests
> at all).
If the setup/teardown fails, I want that to be in the log, so therefore I want that event to fire.  I *only* want to hide setup/teardown messages when they are passing.  Otherwise, you could get into the situation where something is failing and you have no indication why/how/what.  

As for setting the variable, I tried not doing that, but it involves negating that entire if statement or else doing a "if () pass else: <dosmoething> construct.  This way felt the more readable of the two even though it is more verbose.

Comment 5

[cmtalbert]

Landed: https://github.com/mozautomation/mozmill/commit/7b0fd7b55e4308135145d3080a3710564d417e96