feat(logger): Add log level configuration (#1451)

[bcaudan]

No description provided.

[bcaudan]

Is there anything I could do to help this PR to be merged?

[NickTomlin]

We should add entries to the
allowedNames and describes sections of cli.ts.

Otherwise this looks good

[NickTomlin]

What is (<any>LogLevel) accomplishing here? I'm able to compile without it (that said, I'm not a typescript expert, so perhaps I am missing something).

[NickTomlin]

NVM this is an error in [email protected] but not 2.0.10 (which we have pinned)

[bcaudan]

I guess you figured out but it is used to convert string to enum, found on SO.

Would you prefer a comment to make it easier to read or just remove it for now?

[NickTomlin]

I think i'm okay leaving it as is; unless others have strong opinions.

[Xotabu4]

We might try something like this for log levels :

let logLevel: 'ERROR'|'WARN'|'INFO'|'DEBUG'
logLevel = 'OLOLO' //compilation error here

[bcaudan]

I have updated cli.ts accordingly

[NickTomlin]

Looks good to me! Thanks!

@cnishina @seanmay any thoughts?

[NickTomlin]

I was playing around with the typing for LogLevel a little more (using TS 2.1) and I think i've found a simplified version that has the benefit of explicitly stating the values for logLevel in code NickTomlin@3fe8606. This results in an exception in the logger if the value is not valid:

./bin/protractor example/conf.js --logLevel=NOT_VALID
/Users/ntomlin/workspace/protractor/built/logger.js:130
                throw new Error('Log level invalid or undefined');
                ^

Error: Log level invalid or undefined
    at Logger.log_ (/Users/ntomlin/workspace/protractor/built/logger.js:130:23)
    at Logger.debug (/Users/ntomlin/workspace/protractor/built/logger.js:86:14)
    at Object.initFn [as init] (/Users/ntomlin/workspace/protractor/built/launcher.js:98:12)

[bcaudan]

I have updated the PR with @NickTomlin and @Xotabu4 suggestions

[bcaudan]

How are we on this PR?

[NickTomlin]

@seanmay mind taking a look at this when you have some free time?

[igniteram]

can we merge this PR? It is a useful feature!

[qiyigg]

Could you please add a test for this change?

[alecmerdler]

We would also really like to see this merged.

[igniteram]

@bcaudan If you get time can you please add some tests? This PR is pending from long time!
@alecmerdler Meanwhile you could use cliptor for configuring this log level, It basically wraps the same setting which is implemented in this PR.

[alecmerdler]

I've been using this hacky solution for now.

Also, I would be open to writing tests for this PR (or opening another with tests) if that would be okay with @bcaudan.

[qiyigg]

Sorry for the long delay.
I think this PR was pending for a long time and @alecmerdler or someone else, please feel free to create a new one for it.
As long as the new one is created, I will close this one, Thanks!

[alecmerdler]

@qiyigg This PR doesn't have any merge conflicts, why do we need to open a new one? I am glad to do it but would prefer to give @bcaudan the credit.

[bcaudan]

@qiyigg I have added some unit tests for the Logger.
Let me know if you had other tests in mind.

circle ci tests exited with:

A command timed out during your tests

is it a known error? can you trigger a rebuild?

[qiyigg]

@bcaudan I already triggered a rebuild, let's see what happens.
The circleCI should be already fixed since last release.
Thanks for adding these tests. My understanding is it would be better to have a integration test, say, have a separate config file which sets loglevel there, and check it during test. But since it is a simple change, I am OK with current test.
I can merge the change as long as the circleCI is passed.

[alecmerdler]

@qiyigg Will there be a new release soon with these changes?

[qiyigg]

@alecmerdler to be honest, I don't have enough bandwidth recently, I'll try to make it this week, but no guarantee.