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

Include the prices in the tests #463

Merged
merged 36 commits into from
Apr 14, 2021
Merged

Conversation

set-soft
Copy link
Collaborator

This patch allows testing the most important KiCost feature: getting the costs.
To achieve it I'm adding a fake web server that can pretend to be KitSpace's server, but using recorded responses.
In this way we can reproduce the transactions over and over.
The patch also allows to record the transactions and make the currency exchange rates reproducible.

- The envidonment variable KICOST_CURRENCY_RATES can be used to fake
  the cunrrency rates.
- It must indicate the name of an XML file containing the desired
  rates.
- The format is the one used by the European Central Bank.
- This is a very subtle detail. If we import the logger explicitly we
  get a copy of the logger variable. So we can't change it
  "on-the-fly". When using the API from outside we could want to
  replace the original logger by a logger from the calling script.
  So here we use the global logger (not a copy) allowing to the
  calling script to change it.
- Avoid using a dict for QUERY_AVAIABLE_CURRENCIES
- Sort the distributors in the query
- The environment variable KICOST_KITSPACE_URL can be used to define it
- Main purpose is for debugging, using a fake server.
- The environment variable KICOST_LOG_HTTP can be defined to point to
  a file.
- Queries and responses are appended to the file.
- Main purpose is recording the transactions to fake them during debug.
- Now the used currencies are sorted.
- This patch also optimizes the way we collect them.
- Now each netlist file has a separated test. So we can run them
  separately and with different options.
- The date collected from the XML is preserved in the converted CSVs.
  So now we also test the date.
- We always fix the currency rates to known values.
- Added a mechanism to record the KitSpace queries. Just defining
  ADD_QUERY_TO_KNOWN to True.
- Added a dummy web server to fake the KitSpace transactions.
- Enabled the full test for 300-010, acquire-PWM and acquire-PWM_2
@set-soft
Copy link
Collaborator Author

Hi @hildogjr !
This PR is in progress, but I want you to take a look at what I'm doing and tell me your opinion.
The main idea is to test KiCost a little bit better. I think the current regression tests are unacceptable. They are too naive and only a minimal part of the functionality is really tested.
This patch tries to cover more functionality.

set-soft added 16 commits April 10, 2021 11:42
- Python 3.5 inverted them
- Removed unused part_idx
- Commented the query_part_stock_code intention
- Simpliefied the KiCost to KitSpace distributor name translation
- Defined available_distributors
- Avoided a double search on all FIELD_CAT fields
- Clarified when we include manf/manf# info
Now the query works for the dummy and real servers.
Some (unknown) subtle detail forced me to use a dict in the post call
- Makes query_part_info easier to read
- Removes the pseudo-global distributors and currency vars
@set-soft
Copy link
Collaborator Author

At this point the patch seems to be usable.
We just need to extend it to all the tests.
BTW: I don't understand the purpose of each test. It looks like a random collection of fuzzy tests. The tests should be named in a way that we know what's their purpose. Like I did for tests like multipart and variant.
With the current names if a test fails we don't know what it means.

@set-soft
Copy link
Collaborator Author

Question: query_part_info reeceives distributors as argument, but this value is completly ignored.
Even more confusing: FIELDS_CAT (and DISTRIBUTORS) are created from distributor_dict. Then the code computes distributors to remove the distributors not supported by KitSpace API. But the query creation is done with the first two.
Is quite confusing.

- Now we compute an MD5 instead of a random hash for local parts.
- Also added the missing logs
- Now we log stderr and stdout separated
- Detected when asked for prices of the subparts test.
Missing blank lines to separate functions.
- Now all tests support price.
- We run a few tests with --no_price because they don't have any part
  number information.
@set-soft set-soft marked this pull request as ready for review April 13, 2021 20:41
@set-soft
Copy link
Collaborator Author

Hi @hildogjr !
Now the patch seems to be fully functional. All tests can be run with price. I collected 3.2 MiB of queries covering all the tests.
In the process I made the code much more reproducible.
Note that 32bbfca fixes a problem with subparts.
I made a lot of changes to the KitSpace API, most of them to make the output consistent, but some of them to make the code more clear.
The fake webserver is working quite well, so we can test reproducible values and really fast.

@hildogjr
Copy link
Owner

Question: query_part_info reeceives distributors as argument, but this value is completly ignored.

KiCost have the capability of "create local virtual distributor" ("hard coding" the price into the an specific filed of the part into the schematic, by using DIST_NAME:pricing = 1:USDxx.xx ; 10:USDxxx, which also allow the alpha3 currency name). This feature use the file dist_local_tempalte.py and the function query_part_info(foo) uses the query_part_info name argument as this virtual distributor name.

This is an attempt to generate a general distributors template #386: dist_local_template.py and api_partinfo_kitspace.py are in use, api_octopart.py may work but need code change at line 95 of kicost.py to call (in the road map of KiCost I would like to make the new APIs/scrap/local module addition more transparent to programmers, see line 37 of distributors/__init__.py). api_octopart.py was kept from older version to users that have big server request quantity (it was the first API written, before Kitspace).

Even more confusing: FIELDS_CAT (and DISTRIBUTORS) are created from distributor_dict. Then the code computes distributors to remove the distributors not supported by KitSpace API. But the query creation is done with the first two.
Is quite confusing.

If I remember correctly this was just a "band aid" fix, removing it was necessary to not create errors on spreadsheet creation.

@set-soft thanks by the wonderful improvements on test and code revisions, I am checking your work now. @devbisme / @xesscorp (KiCost's creator) may consider grant so maintenance privileges of the repository to @set-soft.

@hildogjr hildogjr merged commit cca98d8 into hildogjr:master Apr 14, 2021
@xesscorp
Copy link
Collaborator

I have invited @set-soft to be a collaborator. Thanks for making these improvements, and thanks to @hildogjr for continuing to drive the project forward.

set-soft added a commit that referenced this pull request Apr 19, 2021
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.

3 participants