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

enhancement idea #5

Open
YuxingW opened this issue Oct 12, 2017 · 6 comments
Open

enhancement idea #5

YuxingW opened this issue Oct 12, 2017 · 6 comments

Comments

@YuxingW
Copy link

YuxingW commented Oct 12, 2017

Recently I am using robotbackgroundlogger in my test and found below areas can be enhanced:

  1. if background loggers are created in several places, they are not sharing the message queue for one thread, so it cannot aggregate all logs for one thread. This can be solved by using Singleton instance of background logger.
  2. The BackgroundMessage output is unexpected and not compatible with MainThread output, actually we can use Message in loggerhelper.py directly.
    I made enhancement locally and it works well, would like to create a pull request for review, please see if it is okay.
@pekkaklarck
Copy link
Member

Pull requests are always welcome. Unfortunately I don't have much time I could spend with this project, so I cannot make promises when it could be reviewed.

@YuxingW
Copy link
Author

YuxingW commented Oct 12, 2017

I was unable to check in my change to my branch:
c:\github\robotbackgroundlogger>git push --set-upstream origin yuxing_dev
Username for 'https://github.com': YuxingW
Password for 'https://[email protected]':
remote: Permission to robotframework/robotbackgroundlogger.git denied to YuxingW.

Can you please take a look? Thanks!

@pekkaklarck
Copy link
Member

It seems you try to push into this repository. You need to create your own fork first. GitHub has good docs related to that. If you are totally new to GitHub, you can start by reading Robot Framework's contribution guidelines. They don't fully apply to this project, but most of the content is valid.

@YuxingW
Copy link
Author

YuxingW commented Oct 12, 2017

Thanks. I was able to create a pull request with the guideline.
It is a minor change, please help review.

@pekkaklarck
Copy link
Member

pekkaklarck commented Oct 12, 2017

Read this originally in a hurry. Here's what I think about the proposals after reading this again and looking PR #6 :

  1. There should be separate issues and PRs about different enhancements.
  2. I don't want to introduce singleton pattern here, at least not so that it is used by default and you cannot create individual instances. Adding a separate class method for that could be considered.
  3. I don't understand your second problem.
  4. As I've already commented, I don't really have time for this project.

@YuxingW
Copy link
Author

YuxingW commented Oct 13, 2017

Thanks.
For 2, I changed it to a get_instance classic method.
For 3, I attached a snapshot of my problem, the first thread log is printed in console instead of ouput.xml, can be solved by using logger and log_message.
Test_Traditional | PASS |
Test_Async_Run *INFO:150
INFO:1507869246033 Default timeout value is :200
INFO:1507869246033 Checking Cached Keys
INFO:1507869246033 Found cached key
INFO:1507869246235 Cleared command line with Ctrl-U
INFO:1507869246235 CLISH session caching enabled
INFO:1507869246235 cmd:[/Manage/License/List]
INFO:1507869246235 menu:[/Manage/License]
INFO:1507869246235 real_cmd:[List]
INFO:1507869246235 Setting clish timeout as:200
| PASS |
5240master nbapp2fb parallel :: A test suite with tests to test Ho... | PASS |
2 critical tests, 2 passed, 0 failed
2 tests total, 2 passed, 0 failed

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

No branches or pull requests

2 participants