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.

Sign up for GitHub

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

Jump to bottom

Adding require for Coverage Conversion and related tests #1472

Merged
merged 1 commit into from
Apr 4, 2017
Merged

Adding require for Coverage Conversion and related tests #1472

merged 1 commit into from
Apr 4, 2017

Conversation

devin-petersohn
Copy link
Member

Resolves #1471

@coveralls
Copy link

coveralls commented Apr 4, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.002%) to 81.535% when pulling f6e726f on devin-petersohn:issue#1471nullpointer into b26b851 on bigdatagenomics:master.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1927/
Test PASSed.

fnothaft
fnothaft requested changes Apr 4, 2017
View reviewed changes
Copy link
Member

@fnothaft fnothaft left a comment

Choose a reason for hiding this comment

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

LGTM, but please switch try/catch to intercept. Thanks @devin-petersohn!

adam-core/src/test/scala/org/bdgenomics/adam/models/CoverageSuite.scala Outdated
.setEnd(2)
.setScore(100)
.build()
try {
Copy link
Member

Choose a reason for hiding this comment

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

Use intercept[IllegalArgumentException] instead of try/catch.

adam-core/src/test/scala/org/bdgenomics/adam/models/CoverageSuite.scala Outdated
.setEnd(2)
.setScore(100)
.build()
try {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

adam-core/src/test/scala/org/bdgenomics/adam/models/CoverageSuite.scala Outdated
.setEnd(2)
.setScore(100)
.build()
try {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

adam-core/src/test/scala/org/bdgenomics/adam/models/CoverageSuite.scala Outdated
.setStart(1)
.setScore(100)
.build()
try {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

adam-core/src/test/scala/org/bdgenomics/adam/models/CoverageSuite.scala Outdated
.setStart(1)
.setEnd(2)
.build()
try {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

@coveralls
Copy link

coveralls commented Apr 4, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.002%) to 81.535% when pulling 7a6d8d9 on devin-petersohn:issue#1471nullpointer into b26b851 on bigdatagenomics:master.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1928/
Test PASSed.

fnothaft
fnothaft requested changes Apr 4, 2017
View reviewed changes
Copy link
Member

@fnothaft fnothaft left a comment

Choose a reason for hiding this comment

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

One small nit! Thanks @devin-petersohn!

adam-core/src/test/scala/org/bdgenomics/adam/models/CoverageSuite.scala Outdated
.build()

val caughtWithEmptyContigName =
intercept[java.lang.IllegalArgumentException](Coverage(featureWithEmptyContigName))
Copy link
Member

Choose a reason for hiding this comment

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

Small nit, but you shouldn't need java.lang. in front of IllegalArgumentException. Would you mind making the change throughout the file?

@devin-petersohn
Adding require for Coverage Conversion and related tests
690f948 
Addressing reviewer comments

Addressing reviewer comments
@coveralls
Copy link

coveralls commented Apr 4, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.002%) to 81.535% when pulling 690f948 on devin-petersohn:issue#1471nullpointer into b26b851 on bigdatagenomics:master.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1929/
Test PASSed.

@fnothaft fnothaft merged commit 98d2c92 into bigdatagenomics:master Apr 4, 2017
@fnothaft
Copy link
Member

fnothaft commented Apr 4, 2017

Merged! Thanks @devin-petersohn!

@heuermh heuermh added this to the 0.23.0 milestone Dec 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fnothaft fnothaft fnothaft approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.23.0
Development

Successfully merging this pull request may close these issues.
5 participants
@devin-petersohn @coveralls @AmplabJenkins @fnothaft @heuermh