[jira] Commented: (HARMONY-691) [classlib][nio] Remove some unstable tests from o.a.h.tests.java.nio.channels.SocketChannelTest.

["Andrew Zhang (JIRA)" <ji...@apache.org>]

    [ http://issues.apache.org/jira/browse/HARMONY-691?page=comments#action_12442160 ] 
            
Andrew Zhang commented on HARMONY-691:
--------------------------------------


   [[ Old comment, sent by email on Thu, 29 Jun 2006 09:46:25 +0800 ]]

Hi George,

It depends on network, platform and machine. It sounds strange, but if you
take a look at these tests code, you will know the reason. In fact, if I
remember correctly, one of these tests has made Harmony build system failed.


Let's take one test for example. (The others are similiar. ) Please see my
comments inline.

  public void testReadByteBuffer_NonBlocking_ReadWriteRealTooLargeData()
            throws Exception {
        byte[] serverWBuf = new byte[CAPACITY_64KB];
        byte[] serverRBuf = new byte[CAPACITY_64KB + 1];
        for (int i = 0; i < serverWBuf.length; i++) {
            serverWBuf[i] = (byte) i;
        }
        java.nio.ByteBuffer buf = java.nio.ByteBuffer
                .allocate(CAPACITY_64KB + 1);
        this.channel1.configureBlocking(false);
        this.channel1.connect(localAddr1);
        Socket serverSocket = this.server1.accept();
        assertFalse(this.channel1.isConnected());
        if (tryFinish()) {
            OutputStream out = serverSocket.getOutputStream();
            InputStream in = serverSocket.getInputStream();
            out.write(serverWBuf);
            int count = 0;
            int total = 0;
            while (total < CAPACITY_64KB) {
                count = this.channel1.read(buf);
// Andrew comment 1: count <= 0 is not the right  break condition for non
blocking read operation.
// The reason you passed this test maybe because of your network condition,
or something below trick (not right way).
                if (count <= 0){
                    break;
                }
                total = total + count;
            }
// Andrew comment 2: assertEquals should be placed here instead of
system.err.println().
// The  test passes even if the result is wrong(only with some system.out).
It doesn't make any sense.
// system.err.println is not recommended in test case.
// I could only image it's useful only if the test can not be executed
because of some environment problems.
// But in this case, the result is wrong!
            if (CAPACITY_64KB == total) {
                buf.put((byte) 1);
                buf.flip();
                assertEquals(66051, buf.asIntBuffer().get());
            } else {
                System.err
                        .println("Read fail in capacity64KB,
testReadByteBuffer_NonBlocking_ReadWriteRealTooLargeData not finish.");
            }
            int writeCount = 0;
            writeCount = this.channel1.write(buf);
            assertTrue(CAPACITY_64KB + 1 >= writeCount);
            count = in.read(serverRBuf);
            while (total < CAPACITY_64KB + 1) {
                count = this.channel1.read(buf);
// The same as comment 1
                if (count <= 0){
                    break;
                }
                total = total + count;
            }
// The same as comment 2. If the test acts wrong at comment 2 (
system.err.println above), but not fails above,
// then following assertEquals would fail.
            if (total > 0) {
                assertEquals(total, CAPACITY_64KB);
                for (int i = 0; i < count; i++) {
                    assertEquals((byte) i, serverRBuf[i]);
                }
            } else {
                System.err
                        .println("Read fail in capacity64KB+1,
testReadByteBuffer_NonBlocking_ReadWriteRealTooLargeData not finish.");
            }
        } else {
// The same as comment 2
            System.err
                    .println("connect fail,
testReadByteBuffer_NonBlocking_ReadWriteRealTooLargeData not finish.");
        }
// Andrew comment 3: Following part is unnecessary. This test is testing
about read/write operation ( in fact, each test  should keep small
// and fast, and is recommended to be focused on one function. This test
tests read and write).
// It should be placed in test_close() method.
// In current version SocketChannelTest, the close() method is tested many
times.
        this.channel1.close();
        try {
            assertEquals(CAPACITY_NORMAL, this.channel1.read(buf));
            fail("Should throw ClosedChannelException");
        } catch (ClosedChannelException e) {
            // correct
        }
    }

Therefore, I think there are several serious problems in these tests, which
may potentially cause Harmony system failed.

Do you agree with me?

I suggest remove them from tests, and I'll rewrite function tests for those
removed part.

It's not a small task, so  I plan to repeat following steps until all of
tests in SocketChannelTest are stable.
1. remove bad tests.
2. add function tests for removed part.

I devide SocketChannelTest into three parts:
1. read / write
2. some other system.err/out/println tests
3. others

For each part, I'll raised a pair of seperated JIRAs to solve.
What's your opnion?  Many thanks!

Best regards,
Andrew




-- 
Andrew Zhang
China Software Development Lab, IBM


> [classlib][nio] Remove some unstable tests from o.a.h.tests.java.nio.channels.SocketChannelTest.
> ------------------------------------------------------------------------------------------------
>
>                 Key: HARMONY-691
>                 URL: http://issues.apache.org/jira/browse/HARMONY-691
>             Project: Harmony
>          Issue Type: Test
>          Components: Classlib
>            Reporter: Andrew Zhang
>         Assigned To: George Harley
>         Attachments: nio.diff
>
>
> Hello, 
> I found some unstable tests in o.a.h.tests.java.nio.channels.SocketChannelTest. 
> I'll upload a patch to remove these unstable tests, and plan to add new stable tests later  in a seperated JIRA. 
> Thanks!
> Best regards
> Andrew

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators: http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira