-
Notifications
You must be signed in to change notification settings - Fork 153
Added Testing #858
base: master
Are you sure you want to change the base?
Added Testing #858
Conversation
On pushToAggregate request header "X-OpenRosa-Version -> 1.0" is set twice and give a java Error under aggregate cause it parsing it as double "1.0, 1,0"
This reverts commit 8449c3e.
…a repeatable entity (getodk#856)" This reverts commit 9bffa68.
This reverts commit 0d8e741.
…es into a repeatable entity (getodk#856)"" This reverts commit 8c18a1a.
@@ -27,4 +29,18 @@ public void regression_wrong_form_name_when_creating_keys_from_briefcase_form_de | |||
FormKey key2 = FormKey.from(new FormStatus(BriefcaseFormDefinition.resolveAgainstBriefcaseDefn(formFile.toFile(), false, briefcaseFolder.toFile()))); | |||
assertThat(key1, is(key2)); | |||
} | |||
|
|||
@Test | |||
public void different_and_same_key_check_if_equal() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are my thoughts about this test:
FormKey
is a value object, so we should assert whether an instance is or is not another instance.
Language, in this case, is important because in Java, being equal to something and "is"-ing that something implies different equality rules.
In this case we want to verify that objects are effectively the same, as if they were primitive values.- I wouldn't bother asserting the
null
case - The mixed types case is formally wrong because you're only matching it against a String, which doesn't probe it won't be equal to any other infinite types that you could use for that assertion. I'd remove that assertion.
- Also, in this codebase we use Hamcrest matchers with
assertThat
. it's important to follow conventions and coding standards. In this case, I'd expect to haveassertThat
assertions instead ofassertFalse
orassertTrue
. - Another remark about coding is the mix of statically imported
assertTrue
versus the fully qualifiedAssert.assertFalse
. I'd expect everything statically imported, or fully qualified. Not a mix.
@Test | ||
public void gets_manifest_url_from_remoteform() { | ||
FormStatus form = new FormStatus(null); | ||
String supposedManifestUrl = "manifest/test/url"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small language tip: Use expected
instead of supposed
as in expectedManifestUrl
.
@@ -43,4 +45,14 @@ public void knows_how_to_build_paths_to_assets_inside_the_storage_directory() { | |||
); | |||
} | |||
|
|||
@Test | |||
public void gets_manifest_url_from_remoteform() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test feels like it's not bringing any value. It doesn't help to describe this the class' behavior with a meaningful example. It doesn't helping prevent bugs either because it only asserts about its initial state.
Test coverage is not a goal by itself, nor it shoule be considered a metrics to get a sense of the quality of a codebase.
Code, on the other hand, is inventory and has inherent cost. That's why I would suggest not writing this test.
public class AggregateAttachmentTest { | ||
@Test | ||
public void computes_the_string_MD5_hash_of_a_file() throws URISyntaxException, IOException { | ||
Path file = Paths.get(AggregateAttachmentTest.class.getResource("/org/opendatakit/briefcase/pull/aggregate/lorem-ipsum-40k.txt").toURI()); | ||
String expectedHash = "E79C0D4AD451003BA8CFDC1183AC89E9"; | ||
assertThat(AggregateAttachment.md5(file), Matchers.is(expectedHash)); | ||
} | ||
|
||
@Test | ||
public void gets_download_url() throws URISyntaxException, MalformedURLException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public void gets_download_url() throws URISyntaxException, MalformedURLException { | |
public void gets_download_url() throws MalformedURLException { |
UriSyntaxException
is not being thrown by this block
public class AggregateAttachmentTest { | ||
@Test | ||
public void computes_the_string_MD5_hash_of_a_file() throws URISyntaxException, IOException { | ||
Path file = Paths.get(AggregateAttachmentTest.class.getResource("/org/opendatakit/briefcase/pull/aggregate/lorem-ipsum-40k.txt").toURI()); | ||
String expectedHash = "E79C0D4AD451003BA8CFDC1183AC89E9"; | ||
assertThat(AggregateAttachment.md5(file), Matchers.is(expectedHash)); | ||
} | ||
|
||
@Test | ||
public void gets_download_url() throws URISyntaxException, MalformedURLException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is not bringing much to the table. The tested method is being tested indirectly for other tests in higher levels of abstraction. I'd suggest removing it.
@Test | ||
public void checks_equality_of_attachment() throws URISyntaxException, MalformedURLException { | ||
AggregateAttachment testAttachment = AggregateAttachment.of("file_1", "123456", "file://org/opendatakit/briefcase/pull/aggregate/lorem-ipsum-40k.txt"); | ||
AggregateAttachment testAttachment2 = AggregateAttachment.of("file_1", "123456", "file://org/opendatakit/briefcase/pull/aggregate/lorem-ipsum-40k.txt"); | ||
AggregateAttachment testAttachment3 = AggregateAttachment.of("file_3", "1234567", "file://org/opendatakit/briefcase/lorem-ipsum-40k.txt"); | ||
assertFalse(testAttachment.equals(null)); | ||
assertTrue(testAttachment.equals(testAttachment)); | ||
assertTrue(testAttachment.equals(testAttachment2)); | ||
assertFalse(testAttachment.equals(testAttachment3)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Test | |
public void checks_equality_of_attachment() throws URISyntaxException, MalformedURLException { | |
AggregateAttachment testAttachment = AggregateAttachment.of("file_1", "123456", "file://org/opendatakit/briefcase/pull/aggregate/lorem-ipsum-40k.txt"); | |
AggregateAttachment testAttachment2 = AggregateAttachment.of("file_1", "123456", "file://org/opendatakit/briefcase/pull/aggregate/lorem-ipsum-40k.txt"); | |
AggregateAttachment testAttachment3 = AggregateAttachment.of("file_3", "1234567", "file://org/opendatakit/briefcase/lorem-ipsum-40k.txt"); | |
assertFalse(testAttachment.equals(null)); | |
assertTrue(testAttachment.equals(testAttachment)); | |
assertTrue(testAttachment.equals(testAttachment2)); | |
assertFalse(testAttachment.equals(testAttachment3)); | |
} | |
@Test | |
public void checks_equality_of_attachment() throws URISyntaxException, MalformedURLException { | |
AggregateAttachment attachment1 = AggregateAttachment.of("111111.txt", "111111", "http://foo.bar/111111.txt"); | |
AggregateAttachment attachment1Clone = AggregateAttachment.of("111111.txt", "111111", "http://foo.bar/111111.txt"); | |
AggregateAttachment attachment2 = AggregateAttachment.of("222222.txt", "222222", "http://foo.bar/222222.txt"); | |
assertThat(attachment1, is(attachment1Clone)); | |
assertThat(attachment1, is(not(attachment2))); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This alternative is has less text to parse, which makes it easier to understand. It also uses consistent numbers and names , and enough assertions to explain what's the behavior we're asserting
|
||
import static org.junit.Assert.*; | ||
|
||
public class RetrieveAvailableFormsFailedEventTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing getters here, which is nor very useful. I'd suggest removing this. test class
|
||
import static org.junit.Assert.*; | ||
|
||
public class ServerConnectionInfoTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tests are only increasing the coverage percentage, which is not useful by itself. This test class doesn't bring much value to the table because equality rules of ServerConnectionInfo are indirectly verified by higher-level of abstraction tests elsewhere. I'd suggest removing this test class
@@ -156,4 +157,12 @@ public void can_handle_3xx_errors() throws Exception { | |||
assertThat(response.isSuccess(), is(false)); | |||
}); | |||
} | |||
|
|||
@Test | |||
public void the_factories_accept_max_http_connections_with_Proxy(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the test method name has to be improved. This test verifies that the returned object is of a certain type, which doesn't have to do with the method name. It's very important that the test methods convey with precission what's being verified.
|
||
@Test | ||
public void the_factories_accept_max_http_connections_with_Proxy(){ | ||
assertEquals(CommonsHttp.class, CommonsHttp.of(MAX_HTTP_CONNECTIONS, null).getClass()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, this verification by itself is not very useful.
@@ -86,4 +89,11 @@ public void can_resolve_paths() { | |||
is(url("http://foo.com/bar/baz")) | |||
); | |||
} | |||
|
|||
@Test | |||
public void can_resolve_body(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test doesn't belong to this test class because what we're verifying is that the RequestSpy
class can read a request body. You don't need a Request
object for that.
|
||
public class TransferFormsTest { | ||
|
||
private List<FormStatus> forms; | ||
private Path briefcaseDir; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private Path briefcaseDir; |
This property is not used. Remove
|
||
@Before | ||
public void setUp() { | ||
briefcaseDir = get("/storage/directory"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
briefcaseDir = get("/storage/directory"); |
Not used. Remove
public void loads_all_forms() { | ||
TransferForms transferForms = TransferForms.empty(); | ||
transferForms.load(forms); | ||
assertEquals(10, transferForms.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not test that the event onChange
is triggered when forms are loaded, which is also part of this method's behavior?
transferForms.getSelectedForms(); | ||
assertEquals(10, transferForms.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
transferForms.getSelectedForms(); | |
assertEquals(10, transferForms.size()); | |
TransferForms selectedForms = transferForms.getSelectedForms(); | |
assertEquals(10, selectedForms.size()); |
This test wasn't doing what you wanted because you weren't using the output of getSelectedForms()
in the assertion.
@@ -57,4 +57,18 @@ public void knows_if_we_are_not_up_to_date() { | |||
|
|||
assertThat(versionManager.isUpToDate(), is(false)); | |||
} | |||
|
|||
@Test | |||
public void version_contains_hyphen(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is useful. However, I think the test method requires a bit of work. Maybe something along the lines "ignores beta version information"
@Test | ||
public void find_meta_tag_successful() { | ||
FormInstanceMetadata metadata = getFormInstanceMetadata(tree); | ||
assertNotNull(metadata); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure you can come up with more interesting assertions than just asserting that the output is not null!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your time!
I think there are some tests that could be merged. See my comments.
As part of a software testing course in our university we are submitting this pull request.
Our first assignment was to try and increase test coverage (especially branch coverage for now) in an open source project.
We are aware that this contribution might not be as useful, but we would be grateful for some feedback :)