Rework custom logger levels

RESOLVED INVALID

Status

Testing Graveyard
Mozmill
RESOLVED INVALID
6 years ago
2 years ago

People

(Reporter: whimboo, Assigned: pranavrc)

Tracking

Details

(Whiteboard: [mentor=whimboo][lang=py][mozmill-2.0])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Currently our logger levels are a bit strange and need to be reworked. Some output will not appear in the levels we would expect. So what we currently have is:

        self.custom_levels = {
            "RESULTS": 1000,
            "TEST-UNEXPECTED-PASS": 43,
            "TEST-UNEXPECTED-FAIL": 42,
            "TEST-SKIPPED": 31,
            "TEST-KNOWN-FAIL": 23,
            "TEST-PASS": 22,
            "TEST-START": 21,
            }

That means that "TEST-UNEXPECTED-FAIL" would not fall into "ERROR", which is bad. :/

Here my proposal:

* "RESULT" we leave as is
* "TEST-UNEXPECTED-PASS" and "TEST-UNEXPECTED-FAIL" should be part of "ERROR".
* "TEST-SKIPPED" should be part of "WARNING".
* "TEST-KNOWN-FAIL", "TEST-PASS", and "TEST-START" should be part of "INFO".

So all the affected numbers would have to be decreased, so that they are below the appropriate default level. Does that sound fine?
(Reporter)

Updated

6 years ago
Blocks: 786969
(Assignee)

Updated

6 years ago
Assignee: nobody → prp.1111
Status: NEW → ASSIGNED
(Assignee)

Comment 1

6 years ago
Created attachment 663822 [details] [diff] [review]
Reworked levels.
Attachment #663822 - Flags: review?(jhammel)
(Reporter)

Comment 2

6 years ago
Comment on attachment 663822 [details] [diff] [review]
Reworked levels.

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

::: mozmill/mozmill/logger.py
@@ +37,5 @@
>  
>          self.custom_levels = {
>              "RESULTS": 1000,
> +            "TEST-UNEXPECTED-PASS": 33,
> +            "TEST-UNEXPECTED-FAIL": 32,

logging.ERROR is 40, so I think that we should count downwards. This would also apply to the other changes. Jeff, what do you think of?

Comment 3

6 years ago
So I'm actually beginning to think that we should have a list in order from least severe to most severe.  Strictly speaking, the numbers don't really mean anything: only the orders matter.  But that can be done later.  For now, I have no strong preference.

Comment 4

6 years ago
Comment on attachment 663822 [details] [diff] [review]
Reworked levels.

this works for me, unless :whimboo has objections
Attachment #663822 - Flags: review?(jhammel) → review+
(Reporter)

Comment 5

6 years ago
(In reply to Jeff Hammel [:jhammel] from comment #3)
> So I'm actually beginning to think that we should have a list in order from
> least severe to most severe.  Strictly speaking, the numbers don't really
> mean anything: only the orders matter.  But that can be done later.  For
> now, I have no strong preference.

Would you mind to file a new bug with specific details? Not sure what you mean by list and how this should work with the logging module. What we have right now is a list sorted by severity and the items map to the logging constants which are numbers. So further input would be appreciated.

(In reply to Jeff Hammel [:jhammel] from comment #4)
> this works for me, unless :whimboo has objections

I would like to see my mentioned nits addressed. Once done I'm happy to check-in.

Comment 6

6 years ago
> Would you mind to file a new bug with specific details?

Filed bug 793853 . I hope it is clear.  The point is that the numbers don't really matter for our purposes.  All we care about is the order.  That said, it is a nice to have, not a huge win.
(Assignee)

Comment 7

6 years ago
Created attachment 664279 [details] [diff] [review]
patch v2

About the other bug, do the levels have to be reassigned so that they're numerically close to each other?
Attachment #663822 - Attachment is obsolete: true
Attachment #664279 - Flags: review?(hskupin)

Comment 8

6 years ago
(In reply to Pranav Ravichandran [:pranavrc] from comment #7)
> Created attachment 664279 [details] [diff] [review]
> patch v2
> 
> About the other bug, do the levels have to be reassigned so that they're
> numerically close to each other?

bug 793853 implies that we don't care about the numerical values, which I don't think we do.  I don't know if we can map multiple levels to a single numerical value (though if we can, I'm not sure why we don't, though IIRC python logging doesn't like that which is a limitation in its code).  But if we can't, then we mostly want 

a) an ordering of levels
b) a console level that you can set from the command line which that and all higher levels are printed to stdout

TL;DR outside of the fact that the numbers we're assigning to don't really mean anything (they're just a substitute for ordering, the way that we use them) the main purpose of decoupling order from numerical level is familiar to BASIC programmers from way back when:  let's say you have levels 30-40 accounted for and you need to insert a level in that range... If we must have a bijection of integers to levels, you need to rewrite many numbers to do this.  If you just have a list of levels then just put the new level where you want it and you're done.
(Reporter)

Updated

6 years ago
Attachment #664279 - Flags: review?(hskupin) → review+
(Reporter)

Comment 9

6 years ago
Thank you Pranav for your first contribution to the Mozmill project. The changeset is not in our master repository!

https://github.com/mozilla/mozmill/commit/972589136c98dbe9f51f00067646018fc3f86118

Lets do any follow-up discussion about logger levels on the newly created bug. This one is fixed now.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Reporter)

Updated

6 years ago
Whiteboard: [mentor=whimboo][lang=py] → [mentor=whimboo][lang=py][mozmill-2.0]
(Reporter)

Comment 10

6 years ago
As it turns out we made a mistake here. But that shows how complicated those logging levels are! It's hard to understand.

So with this change in place we do no longer see the TEST-START and other log messages when running at the INFO level. :(

I backed this out:
https://github.com/mozilla/mozmill/commit/ab1e9366e77866ec1dfb842236fba4fb07a0e6c5

And sorry again for this wrong assumption.
Resolution: FIXED → INVALID
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.