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

fix/Duration/running time display from the Hummingbot #5499

Merged
merged 20 commits into from
Jan 18, 2023

Conversation

theomaniac
Copy link
Contributor

@theomaniac theomaniac commented Jul 14, 2022

Before submitting this PR, please make sure:

  • Your code builds clean without any errors or warnings
  • You are using approved title ("feat/", "fix/", "docs/", "refactor/")

A description of the changes proposed in the pull request:
When the Hummingbot runs for more than 1 day, the bottom status on the left, displaying the running time, gets messed up.

It fixes the issue described in #5497, also makes the elapsed time more Linux Uptime like

Before:
2022-07-14-20-08-44

After:
2022-07-14-20-22-43

Tests performed by the developer:
Waited 1 day

Tips for QA testing:
Wait 1 day ;)

@MementoRC
Copy link
Sponsor Contributor

MementoRC commented Jul 14, 2022

Do you have screenshots by any chance?

@theomaniac
Copy link
Contributor Author

Yes, they are added in the opened issue, but will add them here too

@rapcmia rapcmia linked an issue Jan 12, 2023 that may be closed by this pull request
@nikspz
Copy link
Contributor

nikspz commented Jan 12, 2023

@theomaniac Could you please fix failing tests? thanks

@theomaniac
Copy link
Contributor Author

theomaniac commented Jan 13, 2023

Have asked multiple times and multiple people, about how those can be fixed.
Got no usable answer.
The test it is failing it should be redone.

@theomaniac
Copy link
Contributor Author

Have anybody looked at the tests it is failing and why ?

@cardosofede
Copy link
Contributor

Hi @theomaniac. As I told you before, when you make a change in the code you have to also adapt the test for that.
If you don't know how to do it, another dev can pick the bounty.

@MementoRC
Copy link
Sponsor Contributor

MementoRC commented Jan 15, 2023

@theomaniac Please, change:
"dev dir"/test/hummingbot/client/ui/test_interface_utils.py - lines 48-49 with the updated display you are proposing (Uptime: ...days...)

        self.assertEqual('Duration: 0:00:02', mock_timer.log.call_args_list[0].args[0])
        self.assertEqual('Duration: 0:00:03', mock_timer.log.call_args_list[1].args[0])

It is necessary and very helpful to reviewers and co-developers to implement the appropriate tests to validate the changes made in the PR - modifying existing tests, adding extra tests, removing obsolete tests, etc...
Tests and coverage (which ensure that new code have test evaluating them) can be run with make test before Commit&Push

Hope this helps, thank you for your contribution (I am just a community beginner dev)

@nikspz
Copy link
Contributor

nikspz commented Jan 18, 2023

Hi @theomaniac Would you please answer if you're able update the PR with tests advised above and be assigned for the bounty here?

@theomaniac
Copy link
Contributor Author

Yeah, will look at the tests

@theomaniac
Copy link
Contributor Author

it seems that managed to fix the test, take a look

@nikspz
Copy link
Contributor

nikspz commented Jan 18, 2023

it seems that managed to fix the test, take a look

https://github.com/hummingbot/hummingbot/actions/runs/3949619552/jobs/6761212822
image

@theomaniac
Copy link
Contributor Author

theomaniac commented Jan 18, 2023

fixed, finally it seems that we are getting somewhere

Copy link
Contributor

@nikspz nikspz left a comment

Choose a reason for hiding this comment

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

Tests performed:

  • Cloned and installed feature branch
  • Manually created docker image successfully
  • Ran the longterm bot for 3 days
  • Reviewed that runtime showing correctly and now displayed days

Copy link
Contributor

@cardosofede cardosofede left a comment

Choose a reason for hiding this comment

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

LGTM!

@cardosofede cardosofede merged commit c91a983 into hummingbot:development Jan 18, 2023
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.

Running Duration of the Hummingbot gets messed up after 1 day
7 participants