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

resolved issue #2231 #2245

Merged
merged 1 commit into from
Mar 23, 2016
Merged

resolved issue #2231 #2245

merged 1 commit into from
Mar 23, 2016

Conversation

pollend
Copy link
Member

@pollend pollend commented Mar 19, 2016

Contains

This pull request resolves issues relating to the behavior tree test failing.

How to test

run the unit test for logic/behavior.

@GooeyHub
Copy link
Member

Can one of the admins please verify this patch?

@Cervator
Copy link
Member

@GooeyHub: ok to test

Hey @pollend - on IRC you mentioned thinking this looks like a hack, what do you mean?

Pinging @oniatus and @flo to review.

@GooeyHub
Copy link
Member

Refer to this link for build results (access rights to CI server needed):
http://jenkins.terasology.org/job/TerasologyPRs/504/
Hooray Jenkins reported success with all tests good!

@pollend
Copy link
Member Author

pollend commented Mar 19, 2016

Just seemed like a fairly roundabout way to fix a bug, but if it works then it's probably good. :D

@@ -91,6 +92,10 @@ public Status tick(float deltaSeconds) {
LOG.warn("update of Task {} returned invalid state {}", this.getClass(), newStatus);
}
status = newStatus;
if(newStatus == Status.FAILURE || newStatus == Status.SUCCESS) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this better than calling onTerminate() in the corresponding switch cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

It could still be in the running state. Some of the test expect the onTerminate to be called without the state of the node to be in a terminated state. I guess the test can be modified to work in the other case.

@Cervator Cervator merged commit 93c70a3 into MovingBlocks:develop Mar 23, 2016
@Cervator Cervator added this to the v1.0.1 milestone Mar 23, 2016
@Cervator Cervator added the Category: Build/CI Requests, Issues and Changes targeting gradle, groovy, Jenkins, etc. label Mar 23, 2016
@Cervator
Copy link
Member

Went ahead and merged this so PR checks will look less scary, even if maybe this isn't perfect yet. I added a note in #2070 to give it another pass in the future.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Build/CI Requests, Issues and Changes targeting gradle, groovy, Jenkins, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants