[rfc][icedtea-web][policyeditor] Unit tests fix after last PolicyEditor async IO patch

[Andrew Azores]

On 06/03/2014 05:45 PM, Omair Majid wrote:
> * Andrew Azores <aazores at redhat.com> [2014-06-03 17:32]:
>> The test itself isn't doing anything async, it's PolicyEditor doing it after
>> the SwingWorker patch.
> Yup. Due to the aysnc nature, however, the test has no idea that
> something is happening. If that thing goes wrong, the test has no way of
> finding out.

Well, it sort of does. It finds out by the editor not containing the 
information that it was supposed to have found in the policy file.

>
>> The tests used to expect that after instantiating a
>> PolicyEditor, once the constructor returned, the editor's model of the
>> policy file would already be ready. Now that the file loading is done async
>> that assumption doesn't hold, and it's possible for the editor to not have
>> parsed the policy file by the time the tests run.
> Right, except it there are two things that happening when this test is
> run that are a bad idea for a unit test:
>
> - Something is happening async, that the test can not see. Can you allow
>    the test to inject some sort of a runner, for example? The test could
>    pass a fake thing that causes it to run-in-current-thread, while
>    outside the test, it would run in a different thread.

I don't think I'm really on board with adding that kind of additional 
complexity to it just so that this one method can be tested "better". If 
something goes wrong with the async file IO then the tests already fail. 
If something goes wrong with the actual parsing there are other tests to 
catch it. I don't really see what adding this functionality gives us, 
other than maybe a *very* slightly more specific detail on the failure 
(eg "failed due to this particular IOException while reading from the 
file" rather than "the test that ensures that opening a file with known 
contents results in not having the correct contents in the model, so it 
must not have been read correctly").

>
> - GUI is being invoked. This may interrupt the user and otherwise make
>    things less synchronous (which makes them less reproducible).

GUI actually isn't being invoked, really. Nothing is ever set visible, 
at least, and the editor panel is never even placed into a Window.

>
>> There are already tests for the parsing stage on its own, so
>> extracting out the logic into a tests-accessible Runnable would mostly
>> just be duplicating the standalone parser tests
> You don't have to test the parsing is correct, but it's important to
> verify that it happened and that no errors were thrown (for example).
>
> Thanks,
> Omair
>

As above, it already does this. The reason the tests sporadically fail 
without this patch is because the parsing didn't happen (on time).

Thanks,

-- 
Andrew A