soartech / jsoar

Pure Java implementation of the Soar cognitive architecture.
http://soartech.github.com/jsoar/
BSD 3-Clause "New" or "Revised" License
53 stars 19 forks source link

Possible bug in ThreadedAgentTest #129

Closed msche closed 3 years ago

msche commented 3 years ago

I have forked the JSoar project for some private experimenting and noticed that the build took a long time (5 minutes). So I started looking into the tests and noticed that ThreadedAgentTest.testMultipleAgents() took a long time (> 2 minutes). I was wondering why so investigated the method a bit and noticed the following. At a certain stage the tests verifies whether the agents are running (see below snippet)

// Make sure the agents are running
// If the agents are unhappy or unresponsive, we will timeout in this
// loop
while (!agents.isEmpty())
{
    Iterator<ThreadedAgent> iter = agents.iterator();
    while (iter.hasNext())
    {
        ThreadedAgent ta = iter.next();
        // If the agent successfully stopped, remove it from the list
        if (!ta.isRunning())
        {
            logger.debug("Agent is running: {}", ta.getName());
            iter.remove();
        }
    }
}

To me it seems the if check is wrong. I would have expected that it would check whether the ThreadedAgent is running and NOT whether it was stopped. So I changed that part of the code into:

// Make sure the agents are running
// If the agents are unhappy or unresponsive, we will timeout in this
// loop
List<ThreadedAgent> startedAgents = new ArrayList<>(agents);
while (!startedAgents.isEmpty())
{
    Iterator<ThreadedAgent> iter = startedAgents.iterator();
    while (iter.hasNext())
    {
        ThreadedAgent ta = iter.next();
        // If the agent successfully stopped, remove it from the list
        if (ta.isRunning())
        {
             logger.debug("Agent is running: {}", ta.getName());
             iter.remove();
        }
    }
}

I executed the tests after this modification and they where successful and a lot faster.

Is this actually a bug or did I misunderstood the test.

marinier commented 3 years ago

Thank you for finding and fixing this! I have incorporated the change into the code in b11b5dc1d949c65756afe90382f117028b607750.

It appears that when I updated this test a few months ago I was a bit too hasty and made a copy-paste error and a logical error. I'm surprised the test passed at all given these errors.