Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set log source default level to TRACE. #1244

Merged
merged 7 commits into from
Dec 8, 2018

Conversation

joaonc
Copy link
Contributor

@joaonc joaonc commented Nov 27, 2018

Addresses issue #1199

Having the whole DOM in these keywords at the INFO level is too much clutter and causes the size of the logs to increase dramatically.

@joaonc
Copy link
Contributor Author

joaonc commented Nov 27, 2018

@aaltat / @pekkaklarck The tests in this PR are failing (here), but the failures seem unrelated to the PR.. maybe the tests need to be updated?

@aaltat
Copy link
Contributor

aaltat commented Nov 27, 2018

SeleniumLibrary uses also https://github.com/robotframework/statuschecker/ to verify log messages and keyword status from the output.xml. Also some test may initially fail, but are marked as passed with the status checker and vice versa. Based on the Travis it looks like a some of log check fails. Run the tests with the acceptance test runner: python atest/run.py <browser> and fix the tests.

@aaltat
Copy link
Contributor

aaltat commented Nov 27, 2018

Noticed few bugs in the https://github.com/robotframework/SeleniumLibrary/blob/master/atest/README.rst but now it should be fixed.

@joaonc
Copy link
Contributor Author

joaonc commented Nov 27, 2018

Trying to verify that the DOM is not added to the log at INFO level, but bumping into this issue with robotstatuschecker.

Can you think of another way I can check that the DOM is not logged in that step?

The other way I can think is to simply remove the check, but I'd rather check that is not there than not check at all.

@aaltat
Copy link
Contributor

aaltat commented Nov 28, 2018

There is suggestion, in the robotframework/statuschecker#4 how to overcome the problem. Would you need more help?

@joaonc
Copy link
Contributor Author

joaonc commented Nov 29, 2018

Need some help. The new test cases are passing, except this one (Page Should Contain With Custom Log Level TRACE) b/c loglevel TRACE is not working.

Even if I just do Log Whatever TRACE it won't log anything.

Any idea what's going on?
Followed the code and seemed ok.. TRACE should work.
Note that the previous test case (same but at DEBUG level) works as expected.

@pekkaklarck
Copy link
Member

Without looking anything, my guess is that tests are run using the debug level and thus trace messages are ignored. You can use Set Log Level keyword to set it to trace in setup and again back to debug in teardown.

@joaonc
Copy link
Contributor Author

joaonc commented Nov 29, 2018

Yes, that was it tx!
Tests are now passing except one job here. It's weird b/c I run w/ Python 3.6 on my local and works fine.

I wonder if it's b/c it's running on Jython in the test case Create Webdriver Creates Functioning WebDriver, which should pass and doesn't seem related to my changes.

@aaltat
Copy link
Contributor

aaltat commented Nov 30, 2018

Jython fails randomly in Travis, it is annoying but I have not been able to reproduce the problem locally. I did re-trigger the Jython run and lets see how it goes.

@joaonc
Copy link
Contributor Author

joaonc commented Nov 30, 2018

All is green now, thanks! Hope this gets merged for the next SeleniumLibrary release.

Copy link
Contributor

@aaltat aaltat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you changed all the keywords log level. At least the Log Source log level should not have been changed. Also I did not prepare this big change and I will think it over.

src/SeleniumLibrary/keywords/browsermanagement.py Outdated Show resolved Hide resolved
Setting `log_source(...)` back to 'INFO'.
@joaonc
Copy link
Contributor Author

joaonc commented Dec 6, 2018

@aaltat made the fix you requested and now the Jython test is failing again.. can you kick it off again?

did not prepare this big change and I will think it over

Note that it's the same change everywhere and works as discussed in issue #1199
Also, added test cases to make sure the new logging level is as expected.

Sure hope this makes it.. the amount of unnecessary logging at INFO level is causing me to have to log at ERROR (only place where the DOM is not logged), which in turn causes me to miss out on important info and have to re-run tests when they fail 😞

@aaltat
Copy link
Contributor

aaltat commented Dec 8, 2018

I think this is good to go. I need to create new issue, to that it matches to the scope of the PR.

@aaltat aaltat merged commit ed9498d into robotframework:master Dec 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants