[DISCUSS] Should we improve the structure of our Gelly test suite?

[Andra Lungu]

Hello everyone,

The issue reported here: https://issues.apache.org/jira/browse/FLINK-1587
made us -or at least me :) - wonder if the current approach we have towards
testing the graph methods is the best one.

After implementing the quick fix to the bug(check if the vertex.iterator
hasNext and if it doesn't throw a NoSuchElementException), I went ahead and
tried to test it. You basically just needed an edge set with at least one
invalid src vertex id(i.e. that was not in the vertex set).

The test should then have just consisted of a call to outDegrees on the
malformed graph and would have just needed to be preceded by @Test(expect
ed = NoSuchElementException.class). But this works just when you assert in
each test method. We do the assertion in the @After method, as can be seen
here:
https://github.com/apache/flink/blob/master/flink-staging/flink-gelly/src/test/java/org/apache/flink/graph/test/DegreesITCase.java

The hack I tried then looked something like this:
@After
       public void after() throws Exception {
       if(exception == null) {
           compareResultsB
yLinesInMemory(expectedResult, resultPath);
       } else {
           assert(exception.equals(new NoSuchElementException("The edge set
contains an invalid src/target id")));
       }
       }

and the test was:
@Test
   public void testOutDegreesI
nvalidEdgeSrcId() throws Exception {
       /*
                * Test outDegrees() with an edge containing a source id
that does not
                * exist in the vertex DataSet
                */
       final ExecutionEnvironment env =
ExecutionEnvironment.getExecutionEnvironment();

       Graph<Long, Long, Long> graph =
Graph.fromDataSet(TestGraphUtils.getLongLongVertexData(env),
               TestGraphUtils.getLongLongInvalidEdgeData(env), env);

       try {
           graph.outDegrees().writeAsCsv(resultPath);
           env.execute();
       } catch (Exception exception) {
           this.exception = exception;
       }

But even so, computeResultsByLinesInMemory still got called.

I have not seen tests in Flink who support this kind of check, which is why
I brought this subject up for discussion. What would, in your view be the
best approach?

Thanks!
Andra

[Vasiliki Kalavri]

Hi Andra,

not every test has to be a MultipleProgramsTestBase. We're just using this
for our convenience, it's not a rule :-)
If the case you want to test doesn't fit the format of this test, you can
just make a separate standalone test.
I found some examples for testing exceptions in Flink operators in
the org.apache.flink.api.java.operator package, e.g. CoGroupOperatorTest.
Maybe you could do something similar for FLINK-1587?

Cheers,
V.

On 21 February 2015 at 18:00, Andra Lungu <[hidden email]> wrote:

> Hello everyone,
>
> The issue reported here: https://issues.apache.org/jira/browse/FLINK-1587
> made us -or at least me :) - wonder if the current approach we have towards
> testing the graph methods is the best one.
>
> After implementing the quick fix to the bug(check if the vertex.iterator
> hasNext and if it doesn't throw a NoSuchElementException), I went ahead and
> tried to test it. You basically just needed an edge set with at least one
> invalid src vertex id(i.e. that was not in the vertex set).
>
> The test should then have just consisted of a call to outDegrees on the
> malformed graph and would have just needed to be preceded by @Test(expect
> ed = NoSuchElementException.class). But this works just when you assert in
> each test method. We do the assertion in the @After method, as can be seen
> here:
>
> https://github.com/apache/flink/blob/master/flink-staging/flink-gelly/src/test/java/org/apache/flink/graph/test/DegreesITCase.java
>
> The hack I tried then looked something like this:
> @After
>        public void after() throws Exception {
>        if(exception == null) {
>            compareResultsB
> yLinesInMemory(expectedResult, resultPath);
>        } else {
>            assert(exception.equals(new NoSuchElementException("The edge set
> contains an invalid src/target id")));
>        }
>        }
>
> and the test was:
> @Test
>    public void testOutDegreesI
> nvalidEdgeSrcId() throws Exception {
>        /*
>                 * Test outDegrees() with an edge containing a source id
> that does not
>                 * exist in the vertex DataSet
>                 */
>        final ExecutionEnvironment env =
> ExecutionEnvironment.getExecutionEnvironment();
>
>        Graph<Long, Long, Long> graph =
> Graph.fromDataSet(TestGraphUtils.getLongLongVertexData(env),
>                TestGraphUtils.getLongLongInvalidEdgeData(env), env);
>
>        try {
>            graph.outDegrees().writeAsCsv(resultPath);
>            env.execute();
>        } catch (Exception exception) {
>            this.exception = exception;
>        }
>
> But even so, computeResultsByLinesInMemory still got called.
>
> I have not seen tests in Flink who support this kind of check, which is why
> I brought this subject up for discussion. What would, in your view be the
> best approach?
>
> Thanks!
> Andra
>