fog / fog-xenserver

Module for the 'fog' gem to support XENSERVER
MIT License
16 stars 22 forks source link

Drop Ruby 1.8 support and clean up #66

Open HashNotAdam opened 7 years ago

HashNotAdam commented 7 years ago

TODO

Fog::Compute::XenServer::Models::ClassMethods.define_methods has a note about how it could be better designed if not for Ruby 1.8.7 but I couldn't see a test that could explain the use case. Hopefully someone else can help with this.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.07%) to 94.005% when pulling 54a33231f1c12754a627628b50529a6d118aa670 on HashNotAdam:fix/cop into 2a0ba1565b38a144350fd6c6f42c0b2b86a06e6c on fog:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.07%) to 94.012% when pulling afdcf9dd0069497965b387c133f64f79c2125d70 on HashNotAdam:fix/cop into 2a0ba1565b38a144350fd6c6f42c0b2b86a06e6c on fog:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.07%) to 94.012% when pulling afdcf9dd0069497965b387c133f64f79c2125d70 on HashNotAdam:fix/cop into 2a0ba1565b38a144350fd6c6f42c0b2b86a06e6c on fog:master.

plribeiro3000 commented 7 years ago

@HashNotAdam Thats a great PR. I'm really excited to merge it in.

Could you just fix @houndci-bot comments and the one i made regarding the name of the method before i push this forward please?

HashNotAdam commented 7 years ago

NP @plribeiro3000

In reviewing your comments, I've found a bug in load_data which tells me there is no test for the original code so I'll look at that too.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.1%) to 93.951% when pulling b903c05a39f54012a797511356660afd4ff8aab7 on HashNotAdam:fix/cop into 2a0ba1565b38a144350fd6c6f42c0b2b86a06e6c on fog:master.

HashNotAdam commented 7 years ago

@plribeiro3000, when attempting to add a test for the above-mentioned bug, I encountered a few issues.

Firstly, Fog::Compute::XenServer::Real deals with making server connections. I can see there are a few VCR cassettes that exist that were configured for connecting to a local server, however, I was unable to see any documentation regarding how one might start a server for testing. Without a test server, mock, or cassette, I can't be completely certain that there aren't more bugs.

The second issue is related to the first in that I found there is a huge amount of duplication in this class, however, given the lack of tests, I can't safely refactor. As an example, the method I created from existing code (request_get_all_records) is nearly identical to get_records. Also, most of the request files just make a call to @connection.request using the parser Fog::Parsers::XenServer::Base.new.

If you can provide assistance regarding how I could get a suitable test environment or equivalent, I'd be happy to continue refactoring. If not, I'll just clean up what I've already submitted and leave it there.

plribeiro3000 commented 7 years ago

@HashNotAdam Sadly there isn't a documentation regarding testing because even the tests were never finished. Those VCR tests were made using a real XenServer installation on a physical server.

Feel free to let this change off for now so we can merge what you already have and later (another PR) we can tackle this. What do you think?

HashNotAdam commented 7 years ago

Thanks @plribeiro3000, that sounds good; I'm far too nervous about playing with code I have no way of testing. If anyone comes up with a way to produce a test environment in the future, I'd be happy jump back in.

plribeiro3000 commented 7 years ago

@HashNotAdam Can you confirm nothing broke with those changes?

HashNotAdam commented 7 years ago

@plribeiro3000 no, as mentioned above, I know of at least one bug so I need to do a clean up and then I'll let you know when I believe it is good.

TBH, even then, if there are no tests, there is no way to be 100% certain.

plribeiro3000 commented 7 years ago

@HashNotAdam Thats ok to not work. hehehhe We can do a release candidate to be sure. =)

Let me know when we can move forward with this.