EA-141: Fixed randomly failing test

[HelioStrike]

The test AdtServiceComponentTest.test_getVisitsAndHasVisitDuring() failed randomly because of the line:

assertFalse(service.hasVisitDuring(patient, outpatientDepartment, stopDate, now));

where stopDate is some date from 2012. There was no test above this line which created a visit between the 2 dates, hence the test wasn't definitely failing. As the test patient was being fetched from the database, there's no telling if the patient doesn't have a visit between the specified date range(hasVisitDuring() internally fetches inactive visits too!), which caused the test to fail 'randomly'. I wasn't sure what I should be replacing the line with, so I simply removed it. Doing that fixes the issue.

[dkayiwa]

The original author intentionally put that assertion. So just removing it is not the fix.

[HelioStrike]

Got it. I'll start working on an actual fix.

[HelioStrike]

@dkayiwa I added 2 new methods to AdtService, namely getActiveVisits() and hasActiveVisitsDuring().
The line which was causing the test to fail:

assertFalse(service.hasVisitDuring(patient, outpatientDepartment, stopDate, now));

now becomes:

assertFalse(service.hasActiveVisitDuring(patient, outpatientDepartment, stopDate, now));

I also changed the line:

Date now = new Date(System.currentTimeMillis() - 60*1000);

to:

Date now = new Date(System.currentTimeMillis() - 60*1000);

, going back by a minute. I did this because I wasn't sure whether visits ending at 'now' would be considered active. I think going back by a second would work too, but during creation visits timings are precise up to a minute, so I went with that option. I tested the updated code by running mvn clean install around 10 times, but the test passes.

[dkayiwa]

Do you have to add the new methods to fix the randomly failing test?

[HelioStrike]

@dkayiwa I would assume so. I believe the test is trying to check that the number of active visits until the current point in time is 0. Since hasVisitsDuring() returns all visits, I think the new methods would serve the purpose better.

[dkayiwa]

Can you document these params?

[dkayiwa]

The description does not seem to communicate what is reflected by the method name.

[HelioStrike]

@dkayiwa I've updated the comments. Please have another look.

[dkayiwa]

Can you document these params?

[dkayiwa]

@HelioStrike are you still working on this?

[HelioStrike]

@dkayiwa I think its ready and good to go. By documenting, do you mean sticking a little comment to the right of the @param lines. If so, I'm not sure if I should document the params, because none of the other functions in the file have documented params and it would look a little odd in the file.
Here's the hasVisitDuring() function (present before the changes):

    /**
     * Returns true/false whether or not the patient has any visits at the visit location associated with
     * the specified location during the specified datetime range
     *
     * @param patient
     * @param location
     * @param startDatetime
     * @param stopDatetime
     * @return
     */
    boolean hasVisitDuring(Patient patient, Location location, Date startDatetime, Date stopDatetime);

And here's the new function, hasActiveVisitDuring():

    /**
     * Returns true/false whether or not the patient has any active visits at the visit location associated with
     * the specified location during the specified datetime range
     *
     * @param patient
     * @param location
     * @param startDatetime
     * @param stopDatetime
     * @return
     */
    boolean hasActiveVisitDuring(Patient patient, Location location, Date startDatetime, Date stopDatetime);

All I did was change the description a little to match what the new function was doing. Should I go ahead and put a comment beside each param?

[dkayiwa]

Should I go ahead and put a comment beside each param?

Yes go ahead.