Api/Provider Writing Practices
This section covers practices that are encouraged or discouraged. Reading this section will help guide development in a way that keeps the tenets of jclouds in-tact, and review comments to a minimum.
Start In many cases, we are already practicing things mentioned as Start, just not consistently. Start is mentioned here to keep people on the same page.
Stop Stop means stop propagating, ex. things we should change, but not necessarily backport. For example, we should not employ practice named Stop in any labs api or anything promoted out of labs. However, if something mentioned as Stop exists in a current non-labs api or provider, we should more carefully plan, and possibly allow `Stop` practice to persist for sake of consistency with other code in that api
Undo Undo are unconditionally something we should work on, in any maintained version of jclouds, as these things limit our ability to progress the entire codebase.
General Practice
(Stop) silo'ing fixes!
If you encounter a small problem you need fixed for a downstream api or provider, find some scope to fix uniformly. For example, other classes in the same package, or other methods in the same Api.
Fixing one and leaving the other cleanup for others to address doesn't respect the time of reviewers, or those who merge code at release time. It takes the same time to review a tiny update vs 4 tiny updates. Moreover, a user (or developer) would be unpleasently surprised to find only 1 of 3 edge cases fixed.
When doing bug fixes, or tech-debt removal, can you make it worth it? Could you fix all similar broken windows, or at least more than one? If you can afford time to fix a bug, would spending an extra hour to fix similar ones be tolerable?
Bottom-line, we are all in this together.
(Stop) using Set for everything!
List is the preferred default for a collection, as it maintains order, provides random access, and is a cheap data structure. We've in the past used Set for all collections. Only use Set where it is a very-specific domain concern. Otherwise, use List.
(Undo) putting `@Singleton` on everything!
@Singleton indicates you are holding shared state not given to you in the constructor. Putting @Singleton on everything waters down the concept and slows the runtime of jclouds.
(Stop) Using guava for user-visible types, especially simple ones.
Guava limits our compatibility with users, and limits a future of platforms such as Android. Guava conflicts are very often brought up as reasons why users cannot use latest jclouds. Instead of representing things like `Content-MD5` as a `HashCode`, use the form actually returned by the server (usually a String). Not only does this help with compatibility, but it also prevents us from spending more time re-serializing String->type->String, as well the class of errors that are possible attempting to do so. Stop using `Optional` for example. Just use nullable as that doesn't conflict with Java 8 Optional, Scala Option, etc. Internal use of guava is fine. We should aim to be able to shade jclouds and avoid conflicting on guava completely.
Api interfaces
(Stop) Cargo-Culting aka blindly copy/pasting annotations
There are numerous cases where people have copy/pasted @Fallback annotations which either don't make sense, don't make sense for the api, or actually cause problems. For example, I've seen fallbacks to false on methods that return void, or a list. Also, we fallback on 404 to list methods whose documentation say 404 is not possible. Read the api documentation before making a fallback. If you have a fallback, test it.
Value (Domain) Types
(Start) Using Auto-Value for all value types
Auto-Value covers aspects such as null-checking, hashCode and toString. Use of auto-value reduces the work needed to review and maintain code, and avoids guava incompatibility with things that generate hashCode and toString.
(Stop) Declaring top-level types for subordinate classes.
If you have a status object that's only valid for Server, make it a static inner class of Server. Similarly for single-field types. This makes it easier to understand the top-level, most important types, and prevents us from having to use naming conventions like ServerStatus to disambiguate.
(Undo) Writing builders for output-only types
Just adds work to us and no value to users. Use the canonical factory for that type and name the parameters in your tests.
(Continue) Writing builders for user-input objects with more that 3 parameters.
(Stop) Writing complex builders
Builders will be generated in the future. Stop writing builders with add to collection, convert type functionality, or null checking functionality. Rely on factory methods or auto-value for things like this. Ex. just set the field and continue.
Tests
(Continue) Writing MockWebServer (MockTests)
MockWebServer is used to test that our Api interfaces create expected http requests and parse responses properly. It decouples us from accidentally asserting true-true by using a real http endpoint in tests. Yes, eventhough MockWebServer is named like that, it opens a port and is a real http server!
Use MockWebServer to test all Apis (under the feature package) and any Views (like ComputeService and BlobStore).
MWS basic setup
* Until we switch to JUnit (where we'd use test rules), use a base test class. This should setup a field for the server and tear it down, and hold assets that are shared across tests. This makes the subtypes easier to read and also reduces errors. [Example in GCE](https://github.com/jclouds/jclouds-labs-google/blob/master/google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/internal/BaseGoogleComputeEngineApiMockTest.java).
* Only use MockWebServer's port; Do not use its host. Test hosts in the cloud often botch hostnames. Just point anything that uses the mock server's url. Ex. `"http://localhost:" + server.getPort() + path`.
MWS advanced setup
MockWebServer can do many things, including listen on ssl, spdy, you can run multiple ones to test auth separately, etc. This section covers some non-trivial test concerns.
Handling links in server responses
When your api returns links, you need to correct these to point to the mock server. Otherwise, a compound operation will first go to the Mock server, then go to the real one!
The easiest way is replace well-known base urls with one from the mock server in the base test class.
protected String stringFromResource(String resourceName) {try {return toStringAndClose(getClass().getResourceAsStream(resourceName))
.replace("https://www.googleapis.com/compute/v1/", url("/"));} catch (IOException e) {throw propagate(e);
}
}
MWS do's and don'ts
* Do test annotations you add, especially custom ones. If you add a `@Fallback` test it. Do not add test default fallbacks as this clutters code.
* Do test response parsing, but don't repeat yourself. If you are using a default response parser like json, and you've already a "parse test", just check that the response is the correct type and not null. All you need to do is access the result, as that would imply a ClassCastException if it were the wrong type. Ex. `assertTrue(aggregatedList.instances().hasNext());`
* Test anything "interesting" carefully. Particulary with pagination, test the second page, not just the first. It is "ok" to pin this to a separate class while working on other things, but at least have one class that tests pagination using multiple http responses. For example, in [GCE](https://github.com/jclouds/jclouds-labs-google/blob/master/google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/internal/ToIteratorOfListPageExpectTest.java).
* Don't do multiple "api tests" in the same method. Ex. doing a list followed by a get in the same test method makes the test harder to understand than making different tests. The only reason to load multiple responses is if there's an implicit extra request due to authentication, or there's a continuation like pagination, or you are testing a complex type such as `BlobStore` which needs multiple http requests for the same command.
(Undo) Testing default fallbacks
Tests that cover fallback annotations you didn't add clutters the code, and drowns out important test cases. Only test for code you added.
(Undo) Subclassing BaseRestApiExpectTest
"Expect Tests" were created before we knew about MockWebServer. This is a highly guice-ified, custom way of loading http requests and responses. Due to how resources are loaded, it is possible to get NPEs unless you add a testName attribute to every subclass. Moreover, debugging these has proven extremely difficult. Since ExpectTest mocks an http driver, we have missed bugs in the past due to implementation concerns in ExpectTest. Finally, ExpectTest uses jclouds internals, such as our http request and response objects. As such, any major refactoring to core implies rewriting *all* expect tests. We must stop writing these, and please start converting them.
(Undo) Subclassing BaseRestAnnotationProcessingTest
RestAnnotationProcessor and friends expose the internals of jclouds aging core. It is not possible to simplify core while tests depend on internals. Convert BaseRestAnnotationProcessingTest derivatives to MockTests or delete them.