Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementation and tests #2

Merged
merged 5 commits into from Jun 22, 2016
Merged

Implementation and tests #2

merged 5 commits into from Jun 22, 2016

Conversation

artemik
Copy link
Collaborator

@artemik artemik commented Jun 15, 2016

First pull request.
@mcupak waiting for your review.

@mcupak
Copy link
Owner

mcupak commented Jun 16, 2016

Awesome work @artemik! I really like how well it works when it comes to details, e.g. you can supply beacon IDs as both "-i beacon1,beacon2" and "-i beacon1 -i beacon2". This is a great tool from usability perspective and I'm sure people will be happy to use it.

I stumbled upon the following problems:

  • When I run the tests with mvn test, 5 of them fail with the following error:
Results :

Failed tests:   test(com.dnastack.bob.cli.beacon.BeaconForbiddenTest): No such property: TEST_BEACON_AMPLAB for class: com.dnastack.bob.cli.beacon.BeaconForbiddenTest
  test(com.dnastack.bob.cli.organizations.OrganizationsSuccessTest): No such property: TEST_ORGANIZATIONS for class: com.dnastack.bob.cli.organizations.OrganizationsSuccessTest
  test(com.dnastack.bob.cli.response.ResponseForbiddenTest): No such property: TEST_RESPONSE_AMPLAB for class: com.dnastack.bob.cli.response.ResponseForbiddenTest
  test(com.dnastack.bob.cli.responses.ResponsesAllBeaconsSuccessTest): No such property: TEST_RESPONSES for class: com.dnastack.bob.cli.responses.ResponsesAllBeaconsSuccessTest
  test(com.dnastack.bob.cli.responses.ResponsesForbiddenTest): No such property: TEST_RESPONSES for class: com.dnastack.bob.cli.responses.ResponsesForbiddenTest

Tests run: 13, Failures: 5, Errors: 0, Skipped: 0

I also have the following suggestions:

  • It would be good to add the operation name to the commands. For example: organizations get instread of organizations. We will support more operations in the future, e.g. organization/beacon creation, deletion, updates etc. In fact, the server already supports them, but they're disabled until proper security is added. Granted, for now only a single operation would be supported, so I completely understand why you chose to not have it there. It's just for future extension. This should be accounted for in the help messages (list supported operations etc.).
  • When launching the program without any arguments, which shows the default help message, a non-zero return value should be specified (similarly to other erroneous operations).
  • Some outputs are missing a newline character at the end (help messages as well as regular outputs). This is a Unix convention. Not having that messes up the shell prompt after the run. Examples:
mcupak@... $ java -jar target/cli-1.0-SNAPSHOT-jar-with-dependencies.jar beacon -i amplab
{
  "id" : "amplab",
  "name" : "AMPLab - 1000 Genomes Project",
  "url" : null,
  "organization" : "AMPLab, UC Berkeley",
  "description" : null,
  "homePage" : null,
  "email" : null,
  "aggregator" : false,
  "enabled" : false,
  "visible" : false,
  "createdDate" : 1395273600000,
  "supportedReferences" : [ "HG19", "HG18", "HG38" ],
  "aggregatedBeacons" : null
}mcupak@... $ java -jar target/cli-1.0-SNAPSHOT-jar-with-dependencies.jar beacon
Beacon Network CLI Usage:
 [beacon | beacons | organization |     : Use one of these commands. To get
 organizations | response | responses]    help on each command, type -h with
                                          the corresponding command.
 -u (--url) VAL                         : Beacon Network URL (default =
                                          https://beacon-network.org/api/)
Option "-i (--id)" is requiredmcupak@... $

…de no args execution return fail code, added missing newline characters for cli outputs; rewrote test - added integration tests runs against real beacon network server when specified
@artemik
Copy link
Collaborator Author

artemik commented Jun 20, 2016

@mcupak

#1 issue related changes:

  • fixed tests compilation fail;
  • added inner commands for operations (ex: get, get-all);
  • made empty arguments execution return fail code (help message is still returned);
  • added missing newline character for outputs.

#3 issue related changes:

  • rewrote tests: when "it" profile is used in maven, the tests are run against a real Beacon Network server (the URL is provided by a java property beaconNetwork.test.url, defaults to https://beacon-network.org/api/);
  • changed test data (where needed) for it to match real data, therefore can be used for both mocked and integration tests now.

@mcupak
Copy link
Owner

mcupak commented Jun 22, 2016

Awesome, thanks!

@mcupak mcupak merged commit 68da76a into develop Jun 22, 2016
@mcupak mcupak deleted the impl-and-tests branch June 22, 2016 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants