cloudmesh / cloudmesh-cloud

Multicloud Cloudmesh Plugins for cloudmesh cmd5 CMD
https://cloudmesh.github.io/cloudmesh-manual
Apache License 2.0
2 stars 27 forks source link

Update flavor code, and tests #230

Closed AlexAxthelm closed 4 years ago

AlexAxthelm commented 5 years ago

Updated the code for pulling and parsing EC2 instances, and included a test for parsing.

laszewsk commented 5 years ago

This patch is declined out of two reasons

a) it has a conflict with the current code b) we do not want monkeypatch but we want a real test with real data from aws.

naturally a query needs to be passed along so that a quick response for some data can be generated. The test also introduces several classes so that they seems to be an issue with managing them in a n easy fashion as the other tests demonstrate ...

Furthermore , I am not sure if the comment with using requests has been implemented, e.g. we found previously that urllib* was very inconvenient and had limitations that are overcome by requests.

AlexAxthelm commented 5 years ago

I can resolve the conflict, but I would reccomend keeping the monkeypatch as a test. Actively querying the AWS api introduces an unnecessary point of failure into the test, especially since the actual offer JSON is non-trivial to download (the last one I fetched was over 60 MB).

Testing for JSON responses like this is the simplest way to manage the test, since by writing the the actual response, we can ascertain that the values are being mapped correctly.

The actual JSON fetching still uses requests, like you want.

laszewsk commented 5 years ago

why not do two tests a) one for a simple query that takes seconds b) one for all

you could develop a deactivation for the larger so you could switch it of for smaller testing .... by default its on, you can yous variables = Varirables()

c) we need to do live tests, the mankey patch must be done in a different file such as py_test_aws_fake_test.py as it is a test taht is not related to actively quierying aws as other tests even in other clouds do. Also please be advised that we test in tests/clouds which applies to all clouds

AlexAxthelm commented 5 years ago

Keeping tests small is important, so they run quickly, and people are more likely to actually run them.

Further, the offer file regularly changes. There isn’t a good way to test against the actual offer file, beyond seeing that some data gets populated. There is no way to control ensure that the correct data is populated, without controlling the returned JSON. Writing our own JSON for the rest is the entire point of testing , as it ensures that if we can correctly parse our test data, we are confident that the real data will also parse correctly.

As the test is written now, I’m only monkey patching the requests call. I’m confident that the call will actually work when we need it, since requests is a well-maintained library. Therefore, this represents as full of a test as is possible, while maintaining complete control over the test environment, which is a necessary element of testing.

laszewsk commented 5 years ago

Our tests are used to create benchmarks and tests against live clouds. If you need a monkeypatch test, please create a folder called fake-aws-test and put it in there, great.

laszewsk commented 5 years ago

c) it was revield that a simple test seems not to be possible as the querey interface to images was not implemented. This query interface for example would result in retrieving a single image and woudl fulfill the need of a simple small query. d) it is up to the contributor responsible for adding this functionality to cloudmesh to not only implement a function but to design production level unit tests that work. We identified we need two d1) a benchmark test that returns all images (but does not print them so we can store the benchmark, as storing 94K images in case of a benchmark on aws is not useful. d2) a benchmark test for retrieving a single image from a cloud to showcase that query functionality is implemented and exists.

laszewsk commented 5 years ago

The same argument we made with imageas applies for flavors (e.g. printing all flavors may not be needed, but we need to first find out how the test performs, I have not yet seen that test result from you discussed with us E.g. how many lines are returend in the cloud including test. To showcase that the test can be performed as they are right now all must execute them on opensatck to see if the tests we run can run on more than one cloud.