Bug in assertFileContent function #984

Open
AdamBJ opened this Issue Jan 31, 2017 · 4 comments

Projects

None yet

2 participants

@AdamBJ
Contributor
AdamBJ commented Jan 31, 2017 edited

Brief Description

assertFileContent(), a function critical in the assertUmpleTemplateFor() testing function, fails to detect a mismatch between actual and expected files when the expected file is empty.

Minimum Steps to Reproduce

Look at InterfaceTemplateTest_multipleImplements.java.txt. It's empty, yet is being passed into a template test as expected output in JavaInterfaceTemplateTest.java

Expected Result

An error should be reported since the expected file and actual file don't match (the expected file is empty).

Actual Result

An error is not being reported. The problem is the main expected/actual comparison loop (taken from line 184-204 of SampleFileWriter_Code.ump):

do 
    {
      actualLine = actualReader.readLine();
      if (ignoreLineComments)
      {
        // HACK: To deal with // line # comments
        while (actualLine != null && actualLine.indexOf("// line") != -1)
        { //Ignore the line, go to next
          actualLine = actualReader.readLine();
        }
      }
      expectedLine = expectedReader.readLine();
      
      line++;
      
      // HACK: To deal with umple version issues
      if (expectedLine != null && actualLine != null && expectedLine.indexOf("This code was generated using the UMPLE") == -1)
      {
        Assert.assertEquals("Failed at:" + line,expectedLine,actualLine);  
      }
    } 
    while (expectedLine != null && actualLine != null);

The problem arises when expectedReader is attached to an empty file. In this case, expectedLine is null in our first pass through the loop, which causes us to skip the Assert.assertEquals... check. We simply exit the loop without outputting an error.

@AdamBJ AdamBJ self-assigned this Jan 31, 2017
@AdamBJ
Contributor
AdamBJ commented Jan 31, 2017 edited

1/31/2017: @vahdat-ab @TimLethbridge I've updated this issue, please have another look at the description and the proposed solution and let me know what you think.

Having thought about this issue more carefully, I see that the goal of the loop in question is to check lines of the expected output against corresponding lines of the actual output until there are no more lines of expected output. Doing it this way allows us to only check, say, the first half of the actual output if that's all we're interested in (we just save the first half of the actual output as the expected output).

The problem with this approach is that if we forget to provide expected output (i.e. if expected output is an empty file), the assertUmpleTemplateFor() function doesn't notify us for the reasons mentioned in the description. Therefore, I propose the following to fix this issue:

  1. Add a check to assertFileContent to see if the file provided as expected input is empty. If it is, generate an appropriate error message.

  2. Once 1 is in place, identify all the empty expected test files and populate them with the appropriate data (verifying the data might be a big job, if so I'll split it into a seperate issue).

  3. Add comments to the assertFileContent function to make it's action clearer.

There are 15 JUnit failures that seem to be caused by empty expected output files.

@AdamBJ AdamBJ added Diffic-Med and removed Diffic-Easy labels Feb 1, 2017
@vahdat-ab
Member

can you let me have the name of some of those failed test cases?

@AdamBJ
Contributor
AdamBJ commented Feb 1, 2017 edited

@vahdat-ab Here's a screenshot showing which tests failed. I haven't looked into all of them, but I think they're due to empty files.

image

I started looking into the JavaInheritenceTemplate test cases. It doesn't look like the interface methods are showing up in the classes that implement them (I tried adding a class that extends ISecondChild and didn't see any methods in class body). It also looks like at least one of the interfaces in the .ump file associated with that test contain attributes, which I believe is invalid syntax.

@vahdat-ab
Member

Yes, you are right. When a class implements the interface ISecondChild, there is no method for that. There is a bug in the system. Looks like the code doesn't pay attention to the hierarchy of interfaces.

It also looks like at least one of the interfaces in the .ump file associated with that test contain attributes, which I believe is invalid syntax
Do you mean having attributes in interfaces is an invalid syntax? In Java 8, we can have attributes for interfaces, and it is considered final and static.
However, Umple doesn't still generate attributes for interfaces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment