Skip to content

Conversation

@bmoylan
Copy link
Contributor

@bmoylan bmoylan commented Aug 24, 2016

Fixes #109


This change is Reviewable

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.408% when pulling f5f3636 on bmoylan:patch-1 into 88b78a7 on palantir:develop.

@bmoylan
Copy link
Contributor Author

bmoylan commented Aug 24, 2016

@markelliot

if [[ $? == 0 ]]; then
echo "Process is already running"
exit 1
exit 0
Copy link
Contributor

Choose a reason for hiding this comment

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

test?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.408% when pulling d60f685 on bmoylan:patch-1 into 88b78a7 on palantir:develop.

// try all of the service commands
exec('dist/service-name-0.1/service/bin/init.sh', 'start') ==~ /(?m)Running 'service-name'\.\.\.\s+Started \(\d+\)\n/
file('dist/service-name-0.1/var/log/service-name-startup.log', projectDir).text.contains('Test started\n')
exec('dist/service-name-0.1/service/bin/init.sh', 'start') ==~ /(?m)Process is already running\n/
Copy link
Contributor

Choose a reason for hiding this comment

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

this isn't verifying the return value, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I just ran the test on develop and it passed (so doesn't catch the bug), working on making exec() check the return code. I think there's an assumption throughout this suite that it returns 0 so I can bake in an assertion - you cool with that?

Copy link
Contributor

Choose a reason for hiding this comment

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

bake it into exec()? yeah that sounds ok.

On Wed, Aug 24, 2016 at 12:15 AM Brad Moylan notifications@github.com
wrote:

In
src/test/groovy/com/palantir/gradle/javadist/JavaDistributionPluginTests.groovy
#110 (comment)
:

@@ -61,6 +61,7 @@ class JavaDistributionPluginTests extends GradleTestSpec {
// try all of the service commands
exec('dist/service-name-0.1/service/bin/init.sh', 'start') ==~ /(?m)Running 'service-name'...\s+Started (\d+)\n/
file('dist/service-name-0.1/var/log/service-name-startup.log', projectDir).text.contains('Test started\n')

  •    exec('dist/service-name-0.1/service/bin/init.sh', 'start') ==~ /(?m)Process is already running\n/
    

Yeah I just ran the test on develop and it passed (so doesn't catch the
bug), working on making exec() check the return code. I think there's an
assumption throughout this suite that it returns 0 so I can bake in an
assertion - you cool with that?


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
https://github.com/palantir/gradle-java-distribution/pull/110/files/d60f68511ee7ffd2c83e11735f7a81de586c4c63#r75992572,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGOdweNkSo0licjQCLAaX8qrzeWYCHlBks5qi8VCgaJpZM4Jrmwi
.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@uschi2000
Copy link
Contributor

For reference: http://refspecs.linuxbase.org/LSB_3.1.1/LSB-Core-generic/LSB-Core-generic/iniscrptact.html requires status to return 0 iff "program is running or service is OK".

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.408% when pulling bbae4f6 on bmoylan:patch-1 into 88b78a7 on palantir:develop.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.408% when pulling 6404626 on bmoylan:patch-1 into 88b78a7 on palantir:develop.

@uschi2000
Copy link
Contributor

👍

@markelliot markelliot merged commit ab50be1 into palantir:develop Aug 27, 2016
@bmoylan bmoylan deleted the patch-1 branch September 19, 2016 21:36
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.

4 participants