Added Testing #858

elduwa · 2020-03-24T00:09:26Z

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 :)

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"

…able entity (getodk#856)

…briefcase

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.

ggalmazor · 2020-06-13T13:48:12Z

test/java/org/opendatakit/briefcase/model/form/FormKeyTest.java

@@ -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() {


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 have assertThat assertions instead of assertFalse or assertTrue.

Another remark about coding is the mix of statically imported assertTrue versus the fully qualified Assert.assertFalse. I'd expect everything statically imported, or fully qualified. Not a mix.

ggalmazor · 2020-06-13T13:50:35Z

test/java/org/opendatakit/briefcase/model/FormStatusTest.java

+  @Test
+  public void gets_manifest_url_from_remoteform() {
+    FormStatus form = new FormStatus(null);
+    String supposedManifestUrl = "manifest/test/url";


Small language tip: Use expected instead of supposed as in expectedManifestUrl.

ggalmazor · 2020-06-13T13:52:52Z

test/java/org/opendatakit/briefcase/model/FormStatusTest.java

@@ -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() {


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.

ggalmazor · 2020-06-13T13:53:42Z

test/java/org/opendatakit/briefcase/pull/aggregate/AggregateAttachmentTest.java

 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 {


Suggested change

public void gets_download_url() throws URISyntaxException, MalformedURLException {

public void gets_download_url() throws MalformedURLException {

UriSyntaxException is not being thrown by this block

ggalmazor · 2020-06-13T13:55:15Z

test/java/org/opendatakit/briefcase/pull/aggregate/AggregateAttachmentTest.java

 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 {


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.

ggalmazor · 2020-06-13T13:58:58Z

test/java/org/opendatakit/briefcase/pull/aggregate/AggregateAttachmentTest.java

+  @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));
+  }


Suggested change

@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)));

}

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

ggalmazor · 2020-06-13T14:01:19Z

test/java/org/opendatakit/briefcase/model/RetrieveAvailableFormsFailedEventTest.java

+
+import static org.junit.Assert.*;
+
+public class RetrieveAvailableFormsFailedEventTest {


Testing getters here, which is nor very useful. I'd suggest removing this. test class

ggalmazor · 2020-06-13T14:03:09Z

test/java/org/opendatakit/briefcase/model/ServerConnectionInfoTest.java

+
+import static org.junit.Assert.*;
+
+public class ServerConnectionInfoTest {


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

ggalmazor · 2020-06-13T14:05:30Z

test/java/org/opendatakit/briefcase/reused/http/CommonsHttpTest.java

@@ -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(){


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.

ggalmazor · 2020-06-13T14:06:09Z

test/java/org/opendatakit/briefcase/reused/http/CommonsHttpTest.java

+
+  @Test
+  public void the_factories_accept_max_http_connections_with_Proxy(){
+    assertEquals(CommonsHttp.class, CommonsHttp.of(MAX_HTTP_CONNECTIONS, null).getClass());


Also, this verification by itself is not very useful.

ggalmazor · 2020-06-13T14:07:56Z

test/java/org/opendatakit/briefcase/reused/http/RequestBuilderTest.java

@@ -86,4 +89,11 @@ public void can_resolve_paths() {
        is(url("http://foo.com/bar/baz"))
    );
  }
+
+  @Test
+  public void can_resolve_body(){


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.

ggalmazor · 2020-06-13T14:09:51Z

test/java/org/opendatakit/briefcase/transfer/TransferFormsTest.java


 public class TransferFormsTest {
+
+  private List<FormStatus> forms;
+  private Path briefcaseDir;


Suggested change

private Path briefcaseDir;

This property is not used. Remove

ggalmazor · 2020-06-13T14:10:02Z

test/java/org/opendatakit/briefcase/transfer/TransferFormsTest.java

+
+  @Before
+  public void setUp() {
+    briefcaseDir = get("/storage/directory");


Suggested change

briefcaseDir = get("/storage/directory");

Not used. Remove

ggalmazor · 2020-06-13T14:11:21Z

test/java/org/opendatakit/briefcase/transfer/TransferFormsTest.java

+  public void loads_all_forms() {
+    TransferForms transferForms = TransferForms.empty();
+    transferForms.load(forms);
+    assertEquals(10, transferForms.size());


Why not test that the event onChange is triggered when forms are loaded, which is also part of this method's behavior?

ggalmazor · 2020-06-13T14:12:41Z

test/java/org/opendatakit/briefcase/transfer/TransferFormsTest.java

+    transferForms.getSelectedForms();
+    assertEquals(10, transferForms.size());


Suggested change

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.

ggalmazor · 2020-06-13T14:14:14Z

test/java/org/opendatakit/briefcase/util/BriefcaseVersionManagerTest.java

@@ -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(){


This test is useful. However, I think the test method requires a bit of work. Maybe something along the lines "ignores beta version information"

ggalmazor · 2020-06-13T14:15:18Z

test/java/org/opendatakit/briefcase/util/XmlManipulationUtilsTest.java

+  @Test
+  public void find_meta_tag_successful() {
+    FormInstanceMetadata metadata = getFormInstanceMetadata(tree);
+    assertNotNull(metadata);


I'm sure you can come up with more interesting assertions than just asserting that the output is not null!

ggalmazor

Thanks for your time!

I think there are some tests that could be merged. See my comments.

Marinolino and others added 18 commits March 22, 2020 14:32

Increased Branch Coverage

2d3dfbe

Fix for issue getodk#854 (getodk#855)

17c1c10

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"

Make HTTP post requests repeatable by wrapping entities into a repeat…

223de37

…able entity (getodk#856)

Added Unit Test for XmlManipulationUtils

818ce92

Merge remote-tracking branch 'upstream/master'

1a1e082

Add tests for TranferForms

722ba46

Removed csv report

90ab119

added request body test for RequestBuilder

411c92b

Merge branch 'master' of https://github.com/sw-testing-lab-briefcase/…

f2e8dcf

…briefcase

Increased Branch Coverage in AggregateAttachment

8af120a

Increased branch coverage of FormKey

698b3f2

Increased branch coverage of FormStatus

ddd556f

Covered all branches in BriefcaseVersionManager class

4de5f69

adjustment

8449c3e

Revert "adjustment"

0d8e741

This reverts commit 8449c3e.

Revert "Make HTTP post requests repeatable by wrapping entities into …

8c18a1a

…a repeatable entity (getodk#856)" This reverts commit 9bffa68.

Revert "Revert "adjustment""

076788e

This reverts commit 0d8e741.

Revert "Revert "Make HTTP post requests repeatable by wrapping entiti…

7733176

…es into a repeatable entity (getodk#856)"" This reverts commit 8c18a1a.

ggalmazor reviewed Jun 13, 2020

View reviewed changes

ggalmazor suggested changes Jun 13, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Testing #858

Added Testing #858

elduwa commented Mar 24, 2020

ggalmazor Jun 13, 2020

ggalmazor Jun 13, 2020

ggalmazor Jun 13, 2020

ggalmazor Jun 13, 2020

ggalmazor Jun 13, 2020

ggalmazor Jun 13, 2020 •

edited

Loading

ggalmazor Jun 13, 2020

ggalmazor Jun 13, 2020

ggalmazor Jun 13, 2020

ggalmazor Jun 13, 2020

ggalmazor Jun 13, 2020

ggalmazor Jun 13, 2020

ggalmazor Jun 13, 2020

ggalmazor Jun 13, 2020

ggalmazor Jun 13, 2020

ggalmazor Jun 13, 2020

ggalmazor Jun 13, 2020

ggalmazor Jun 13, 2020

ggalmazor left a comment

	public void gets_download_url() throws URISyntaxException, MalformedURLException {
	public void gets_download_url() throws MalformedURLException {


		import static org.junit.Assert.*;

		public class RetrieveAvailableFormsFailedEventTest {


		import static org.junit.Assert.*;

		public class ServerConnectionInfoTest {

		transferForms.getSelectedForms();
		assertEquals(10, transferForms.size());

Added Testing #858

Are you sure you want to change the base?

Added Testing #858

Conversation

elduwa commented Mar 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggalmazor Jun 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggalmazor left a comment

Choose a reason for hiding this comment

ggalmazor Jun 13, 2020 •

edited

Loading