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

Fixed incorrect data display using stocks/fa/est command #4940

Merged
merged 7 commits into from
May 1, 2023
Merged

Fixed incorrect data display using stocks/fa/est command #4940

merged 7 commits into from
May 1, 2023

Conversation

the-praxs
Copy link
Contributor

Fixes #4938.

get_estimates has been revamped from the ground up to provide correct data for displaying on the terminal.

@reviewpad reviewpad bot added the feat S Small T-Shirt size Feature label May 1, 2023
@the-praxs the-praxs changed the title Hotfix/4938 Fixed incorrect data display using stocks\fa\est command May 1, 2023
@the-praxs the-praxs changed the title Fixed incorrect data display using stocks\fa\est command Fixed incorrect data display using stocks/fa/est command May 1, 2023
@the-praxs
Copy link
Contributor Author

@jmaslek Is the test failing due to some expected values in test_estimates.txt?

@jmaslek
Copy link
Collaborator

jmaslek commented May 1, 2023

@jmaslek Is the test failing due to some expected values in test_estimates.txt?

Yup! Since you changed the output slightly, the test needs to be re-recorded to capture what is expected. You should be able to do so with:

(do for both _model and _view)

pytest tests/openbb_terminal/stocks/fundamental_analysis/test_business_insider_model.py --rewrite-expected

@the-praxs
Copy link
Contributor Author

@jmaslek Is the test failing due to some expected values in test_estimates.txt?

Yup! Since you changed the output slightly, the test needs to be re-recorded to capture what is expected. You should be able to do so with:

(do for both _model and _view)

pytest tests/openbb_terminal/stocks/fundamental_analysis/test_business_insider_model.py --rewrite-expected

I get the following error when I run this command - Exit: Uninstall brotli and brotlipy before running tests

How to solve this issue?

@jmaslek
Copy link
Collaborator

jmaslek commented May 1, 2023

@jmaslek Is the test failing due to some expected values in test_estimates.txt?

Yup! Since you changed the output slightly, the test needs to be re-recorded to capture what is expected. You should be able to do so with:
(do for both _model and _view)
pytest tests/openbb_terminal/stocks/fundamental_analysis/test_business_insider_model.py --rewrite-expected

I get the following error when I run this command - Exit: Uninstall brotli and brotlipy before running tests

How to solve this issue?

pip uninstall Brotli -y

@the-praxs
Copy link
Contributor Author

@jmaslek Is the test failing due to some expected values in test_estimates.txt?

Yup! Since you changed the output slightly, the test needs to be re-recorded to capture what is expected. You should be able to do so with:
(do for both _model and _view)
pytest tests/openbb_terminal/stocks/fundamental_analysis/test_business_insider_model.py --rewrite-expected

I get the following error when I run this command - Exit: Uninstall brotli and brotlipy before running tests
How to solve this issue?

pip uninstall Brotli -y

This worked, thanks for the help!

I have a question - what is the purpose of brotli and brotlipy package in OpenBB?

@jmaslek
Copy link
Collaborator

jmaslek commented May 1, 2023

@jmaslek Is the test failing due to some expected values in test_estimates.txt?

Yup! Since you changed the output slightly, the test needs to be re-recorded to capture what is expected. You should be able to do so with:
(do for both _model and _view)
pytest tests/openbb_terminal/stocks/fundamental_analysis/test_business_insider_model.py --rewrite-expected

I get the following error when I run this command - Exit: Uninstall brotli and brotlipy before running tests
How to solve this issue?

pip uninstall Brotli -y

This worked, thanks for the help!

I have a question - what is the purpose of brotli and brotlipy package in OpenBB?

So Brotli is a compression algorithm (I believe its googles). IN our early days, we had issues with using pytest when the brotli compression was used. So it was not in the dependency tree. It was recently brought in with the video transcription, so in order to avoid issues with tests, we have a check to uninstall it

@codecov
Copy link

codecov bot commented May 1, 2023

Codecov Report

Patch coverage: 44.37% and project coverage change: -0.20 ⚠️

Comparison is base (df9a154) 58.29% compared to head (6ab0f77) 58.09%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4940      +/-   ##
===========================================
- Coverage    58.29%   58.09%   -0.20%     
===========================================
  Files          588      588              
  Lines        53666    53680      +14     
===========================================
- Hits         31283    31185      -98     
- Misses       22383    22495     +112     
Impacted Files Coverage Δ
...inal/core/sdk/controllers/crypto_sdk_controller.py 0.00% <ø> (ø)
...inal/core/sdk/controllers/stocks_sdk_controller.py 0.00% <ø> (ø)
...penbb_terminal/core/sdk/models/crypto_sdk_model.py 0.00% <0.00%> (ø)
...bb_terminal/core/sdk/models/portfolio_sdk_model.py 0.00% <0.00%> (ø)
...penbb_terminal/core/sdk/models/stocks_sdk_model.py 0.00% <ø> (ø)
openbb_terminal/core/sdk/sdk_helpers.py 36.92% <ø> (ø)
openbb_terminal/core/sdk/sdk_init.py 92.75% <ø> (ø)
openbb_terminal/core/sdk/trailmap.py 92.68% <ø> (ø)
openbb_terminal/dashboards/stream/Forecasting.py 0.00% <0.00%> (ø)
...bb_terminal/dashboards/stream/streamlit_helpers.py 0.00% <0.00%> (ø)
... and 19 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jmaslek jmaslek added this pull request to the merge queue May 1, 2023
Merged via the queue into OpenBB-finance:develop with commit 3d7b934 May 1, 2023
@the-praxs the-praxs deleted the hotfix/4938 branch May 8, 2023 04:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat S Small T-Shirt size Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] fa/est -t AAPL -e quarterearnings - data is not returning properly.
2 participants