setup* and teardown* should not be counted as tests

RESOLVED FIXED

Status

Testing Graveyard
Mozmill
RESOLVED FIXED
7 years ago
a year ago

People

(Reporter: Jeff Hammel, Assigned: cmtalbert)

Tracking

Details

(Whiteboard: [mozmill-2.0+])

Attachments

(1 attachment)

WIP
4.07 KB, patch
Jeff Hammel
: review+
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
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
(Reporter)

Updated

7 years ago
Whiteboard: [mozmill-2.0?]
(Assignee)

Updated

7 years ago
Whiteboard: [mozmill-2.0?] → [mozmill-2.0+]
(Assignee)

Comment 1

7 years ago
Created attachment 528406 [details] [diff] [review]
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.
(Assignee)

Comment 2

7 years ago
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
Attachment #528406 - Flags: review?(jhammel)
(Reporter)

Comment 3

7 years ago
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).
Attachment #528406 - Flags: review?(jhammel) → review+
(Assignee)

Comment 4

7 years ago
(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.
(Assignee)

Comment 5

7 years ago
Landed: https://github.com/mozautomation/mozmill/commit/7b0fd7b55e4308135145d3080a3710564d417e96
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Assignee: nobody → ctalbert
Depends on: 885141
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.