facioquo / stock-indicators-python

Stock Indicators for Python. Maintained by @LeeDongGeon1996
https://python.StockIndicators.dev
Apache License 2.0
208 stars 35 forks source link

use CSV test files #303

Closed alexpvpmindustry closed 1 year ago

alexpvpmindustry commented 1 year ago

Description

migrated from samples/quotes/History.xlsx to samples/quotes/*.csv removed dependency of from openpyxl import load_workbook in tests/conftest.py

Fixes #188

Checklist

alexpvpmindustry commented 1 year ago

First of all, thank you for contributing! I love what you're doing here, long overdue.

no problem 😊, just happen to stumble upon this dinosaur fossil 🤣

In addition to the specific comments, I have a general thought that duplicating these CSV files from the source seems a bit icky, but I also think referencing another repo is not a great practice. Can you think of a way to simply use the source CSV files rather than putting them here? The only reason I suggest it is that these tests mirror the originating DLL tests and I'd really only want to maintain these CSV files there, ideally, and not have to worry about syncing it here too.

i see what you meant. I'm not too sure if there is a method for this that works with pytest. but if the testing data doesn't change too often, I think copying the csv files over should probably be good enough.

DaveSkender commented 1 year ago

@LeeDongGeon1996 take a quick peek and merge when ready, LGTM.

DaveSkender commented 1 year ago

One or more of these files aren’t importing correctly, not sure which ones. Unit tests are blowing up.

alexpvpmindustry commented 1 year ago

just to be sure, its in the import stage and not in the testing stage? in the testing stage I get 7 failed unit tests, but I have no idea why they fail image this is the error log for one of the cases image

DaveSkender commented 1 year ago

I think so. My hypothesis is based on all tests passing as of https://github.com/DaveSkender/Stock.Indicators.Python/pull/303/commits/ab5cce92a170a5bad50cfe4a064cd47c25818688 previously.

alexpvpmindustry commented 1 year ago

ok i found the issue, there are several csv that don't follow the current parsing rules. ill fix them

alexpvpmindustry commented 1 year ago

ok fixed! lets wait for the tests to complete

LeeDongGeon1996 commented 1 year ago

LGTM, thanks!

DaveSkender commented 1 year ago

Thanks for contributing @alexpvpmindustry, much appreciated. ❤️