Skip to content
This repository has been archived by the owner on Apr 16, 2022. It is now read-only.

Added Testing #858

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions test/java/org/opendatakit/briefcase/model/FormStatusTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
import static org.junit.Assert.assertThat;

import java.nio.file.Path;
import java.util.Optional;
import org.junit.Assert;
import org.junit.Test;

public class FormStatusTest {
Expand Down Expand Up @@ -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() {
Copy link
Contributor

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.

FormStatus form = new FormStatus(null);
String supposedManifestUrl = "manifest/test/url";
Copy link
Contributor

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.

RemoteFormDefinition remoteForm = new RemoteFormDefinition("name", "1","v1",
"manifest/test/url", "/download/test/url");
Assert.assertEquals(supposedManifestUrl, remoteForm.getManifestUrl());
Assert.assertEquals(Optional.empty(), form.getManifestUrl());
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package org.opendatakit.briefcase.model;

import org.junit.Test;

import static org.junit.Assert.*;

public class RetrieveAvailableFormsFailedEventTest {
Copy link
Contributor

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


@Test
public void getReason_unknown() {
RetrieveAvailableFormsFailedEvent testEvent = new RetrieveAvailableFormsFailedEvent(null);
assertEquals("unknown", testEvent.getReason());
}

@Test
public void getReason_exception() {
Exception e = new Exception("fail");
RetrieveAvailableFormsFailedEvent testEvent = new RetrieveAvailableFormsFailedEvent(e);
assertEquals("Exception: fail", testEvent.getReason());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package org.opendatakit.briefcase.model;

import org.junit.Test;

import static org.junit.Assert.*;

public class ServerConnectionInfoTest {
Copy link
Contributor

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


@Test
public void testEquals_False() {
char[] testPassword = {'a'};
ServerConnectionInfo testInfo = new ServerConnectionInfo("testURL", "testName", testPassword);
Object o = new Object();

assertFalse(testInfo.equals(o));
}

@Test
public void testEquals_True() {
char[] testPassword = {'a'};
ServerConnectionInfo testInfo = new ServerConnectionInfo("testURL", "testName", testPassword);
ServerConnectionInfo testInfo2 = new ServerConnectionInfo("testURL", "testName", testPassword);

assertTrue(testInfo.equals(testInfo2));
}

@Test
public void testEquals_True_Itself() {
char[] testPassword = {'a'};
ServerConnectionInfo testInfo = new ServerConnectionInfo("testURL", "testName", testPassword);
assertTrue(testInfo.equals(testInfo));
}

}
16 changes: 16 additions & 0 deletions test/java/org/opendatakit/briefcase/model/form/FormKeyTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@

import static org.hamcrest.Matchers.is;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;
import static org.opendatakit.briefcase.ui.pull.FormInstallerTest.getPath;
import static org.opendatakit.briefcase.util.StringUtils.stripIllegalChars;

import java.io.IOException;
import java.net.URISyntaxException;
import java.nio.file.Files;
import java.nio.file.Path;
import org.junit.Assert;
import org.junit.Test;
import org.opendatakit.briefcase.model.BriefcaseFormDefinition;
import org.opendatakit.briefcase.model.FormStatus;
Expand All @@ -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() {
Copy link
Contributor

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 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.

FormKey key1 = FormKey.of("Key_1", "1", "v1.0");
FormKey key2 = FormKey.of("Key_1", "2", "v2.0");
FormKey key3 = FormKey.of("Key_1", "1", "v1.0");
String keyString = "key_4";
FormKey keyNull = null;
Assert.assertFalse(key1.equals(key2));
assertTrue(key1.equals(key1));
assertTrue(key1.equals(key3));
Assert.assertFalse(key1.equals(keyString));
Assert.assertFalse(key1.equals(keyNull));
}
}
Original file line number Diff line number Diff line change
@@ -1,19 +1,39 @@
package org.opendatakit.briefcase.pull.aggregate;

import static org.junit.Assert.assertThat;

import java.io.IOException;
import java.net.MalformedURLException;
import java.net.URISyntaxException;
import java.net.URL;
import java.nio.file.Path;
import java.nio.file.Paths;
import org.hamcrest.Matchers;
import org.junit.Test;

import static org.junit.Assert.*;

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

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.

AggregateAttachment testAttachment = AggregateAttachment.of("file_1", "123456", "file://org/opendatakit/briefcase/pull/aggregate/lorem-ipsum-40k.txt");
URL expectedUrl = new URL("file://org/opendatakit/briefcase/pull/aggregate/lorem-ipsum-40k.txt");
assertEquals(expectedUrl, testAttachment.getDownloadUrl());
}

@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));
}
Comment on lines +29 to +38
Copy link
Contributor

@ggalmazor ggalmazor Jun 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

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

}
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import static com.github.dreamhead.moco.Runner.running;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.is;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertThat;
import static org.opendatakit.briefcase.matchers.ExceptionMatchers.throwsException;
import static org.opendatakit.briefcase.reused.http.Http.MAX_HTTP_CONNECTIONS;
Expand Down Expand Up @@ -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(){
Copy link
Contributor

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.

assertEquals(CommonsHttp.class, CommonsHttp.of(MAX_HTTP_CONNECTIONS, null).getClass());
Copy link
Contributor

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.

}



}
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import static org.junit.Assert.assertThat;
import static org.opendatakit.briefcase.reused.http.RequestBuilder.url;

import java.io.InputStream;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
Expand All @@ -29,6 +30,8 @@

public class RequestBuilderTest {

private static final String TEST_BODY = "{\"query\": \"some-text\" }";

@Test
public void can_compose_multiple_path_parts() {
List<String> baseUrls = Arrays.asList("http://foo.com", "http://foo.com/");
Expand Down Expand Up @@ -86,4 +89,11 @@ public void can_resolve_paths() {
is(url("http://foo.com/bar/baz"))
);
}

@Test
public void can_resolve_body(){
Copy link
Contributor

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.

Request<InputStream> request = RequestBuilder.get("http://foo.com").withBody(TEST_BODY).build();
String body = RequestSpy.read(request.getBody());
assertThat(body, is(TEST_BODY));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,34 @@

package org.opendatakit.briefcase.transfer;

import static java.nio.file.Paths.get;
import static java.util.Collections.singletonList;
import static org.hamcrest.Matchers.is;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;

import java.nio.file.Path;
import java.util.ArrayList;
import java.util.List;
import org.junit.Before;
import org.junit.Test;
import org.opendatakit.briefcase.model.FormStatus;
import org.opendatakit.briefcase.model.FormStatusBuilder;
import org.opendatakit.briefcase.model.TestFormDefinition;

public class TransferFormsTest {

private List<FormStatus> forms;
private Path briefcaseDir;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private Path briefcaseDir;

This property is not used. Remove


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Not used. Remove

forms = new ArrayList<FormStatus>();
forms = FormStatusBuilder.buildFormStatusList(10);
}

@Test
public void empty_forms_can_merge_forms() {
// What we are really testing is that no UnsupportedOperationException
Expand All @@ -42,4 +62,27 @@ public void forms_from_varargs_factory_can_be_cleared() {
forms.clear();
assertThat(forms.isEmpty(), is(true));
}

@Test
public void loads_all_forms() {
TransferForms transferForms = TransferForms.empty();
transferForms.load(forms);
assertEquals(10, transferForms.size());
Copy link
Contributor

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?

}

@Test
public void test_select_all() {
TransferForms transferForms = TransferForms.from(forms);
transferForms.selectAll();
assertTrue(transferForms.allSelected());
assertTrue(transferForms.someSelected());
}

@Test
public void get_selected_forms() {
TransferForms transferForms = TransferForms.from(forms);
transferForms.selectAll();
transferForms.getSelectedForms();
assertEquals(10, transferForms.size());
Comment on lines +85 to +86
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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(){
Copy link
Contributor

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"

FakeHttp http = new FakeHttp();

http.stub(
get(url("https://api.github.com/repos/opendatakit/briefcase/releases/latest")).build(),
ResponseHelpers.ok("{\"tag_name\":\"v2.0.0\"}")
);

BriefcaseVersionManager versionManager = new BriefcaseVersionManager(http, "v2.0.0-test");

assertThat(versionManager.isUpToDate(), is(true));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
package org.opendatakit.briefcase.util;

import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
import static org.opendatakit.briefcase.util.XmlManipulationUtils.*;

import org.junit.Before;

import org.junit.Test;
import org.kxml2.kdom.Element;
import org.kxml2.kdom.Node;

public class XmlManipulationUtilsTest {

private static final String ROOT_URI = "http://www.w3.org/2002/xforms";
private static final String OPEN_ROSA_NAMESPACE = "http://openrosa.org/xforms";
private static final String OPEN_ROSA_METADATA_TAG = "meta";
private static final String OPEN_ROSA_INSTANCE_ID = "instanceID";
private static final String NAMESPACE_ATTRIBUTE = "xmlns";
private Element tree;

@Before
public void setUp(){
tree = new Element().createElement(null, "html");
tree.setAttribute(null, NAMESPACE_ATTRIBUTE, ROOT_URI);
Element head = new Element().createElement(null, "head");
Element title = new Element().createElement(null, "title");
Element model = new Element().createElement(null, "model");
Element instance = new Element().createElement(null, "instance");
Element data = new Element().createElement(null, "data");
Element meta = new Element().createElement(OPEN_ROSA_NAMESPACE, OPEN_ROSA_METADATA_TAG);
Element instanceId = new Element().createElement(OPEN_ROSA_NAMESPACE, OPEN_ROSA_INSTANCE_ID);
tree.addChild(Node.ELEMENT, head);
head.addChild(Node.ELEMENT, title);
head.addChild(Node.ELEMENT, model);
model.addChild(Node.ELEMENT, instance);
instance.addChild(Node.ELEMENT, data);
data.addChild(Node.ELEMENT, meta);
meta.addChild(Node.ELEMENT, instanceId);
}

@Test
public void find_meta_tag_successful() {
FormInstanceMetadata metadata = getFormInstanceMetadata(tree);
assertNotNull(metadata);
Copy link
Contributor

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!

}

}