-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Fix Test by use report file as bytes #1685
Conversation
Could you explain the reasoning behind the change in the shutdown handling (replacing the |
I believe I've fixed this in cb56d64 (I had to get all the tests to pass when migrating CI from Travis to Github Actions). Could you confirm if that fixes your issue? |
It was to ensure that the code for html_file option will be always reachable because in some scenarios it wasn't running at all and I think that it's the same logic but with more simple code. |
…s-execution � Conflicts: � locust/main.py
@@ -417,17 +417,15 @@ def shutdown(): | |||
# install SIGTERM handler | |||
def sig_term_handler(): | |||
logger.info("Got SIGTERM signal") | |||
shutdown() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, won't removing this call stop shutdown()
from being called when the process receives a TERM
signal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe I've fixed this in cb56d64 (I had to get all the tests to pass when migrating CI from Travis to Github Actions).
Could you confirm if that fixes your issue?
I can confirm that the issue was fixed proper with your change. It's almost the same code. I have updated this PR in order to be able to see the final changes. But I think that it could be closed if you don't want to merge the change of the finally
. Let me know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the finally clause will warranty the execution of the shutdown method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could be wrong, but I don't think that the main_greenlet.join()
will return unless shutdown() is called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You aren't wrong... but I think that in order to warranty the usage of the html report option the finally need to be used or like you write below move the code to the shutdown method will work too
Unrelated to this PR, but looking at the code now, I think that the |
The update of Echarts contains ascii characters that provoke errors when saving the html file with the concatenated content containing those ascii characters.
To solve this I have changed the file to handle bytes
Results of the changes:
I have been working with this thing of tests failing a little bit and this are my results
Only py37 tests are failling but it's due to that it's not installing the python dependencies with errors like this:
py36, py38 and py39 logs are good, like: