Go Tool for ReviewBot

Review Request #11240 — Created Oct. 23, 2020 and updated

jblazusi
ReviewBot
master
8ab7b4c...
reviewbot, students
ceciliawei, jace

This tool's ability to format was removed and placed into another tool,
that tool is tentatively known as the GofmtTool. This was done because,
go fmt is rather lightweight and does not rely on other files in order
to carry out its function. However, go test and go vet require more
information in order to work consistently. In particular they need
access to the entire repository so that patched code can be fully
analyzed against the package code. Therefore GofmtTool inherits from
Tool, whereas GoTool inherits from RepositoryTool and is generally
a much more computationally heavy tool.

Manual testing was done to confirm that go test is working correctly.
As of now it just creates general comments based on which tests have
failed.
Manual testing was done to confirm that go vet is working and known
issues about failures when used against test files have been resolved.

Loading file attachments...

Description From Last Updated

Go/Go Tools?

daviddavid

gofmt still exists, but I believe the modern recommendation is to use go fmt. Can we do that here to ...

daviddavid

Perhaps "This file contains formatting errors and should be run through go fmt" ? Also, shouldn't this be checking the ...

daviddavid

Since we have the output already in a variable, it's kind of silly to write it out to a file ...

daviddavid

The imports should be in alphabetical order. So import json should be placed before import logging.

ceciliaweiceciliawei

Using the string's "split" method isn't portable (for example, windows doesn't use "/" as a path splitter). That said, there's ...

daviddavid

It looks like we're running this for every single file, which means that if you have multiple changed files in ...

daviddavid
jblazusi
david
  1. 
      
  2. bot/reviewbot/tools/gotool.py (Diff revision 2)
     
     

    Go/Go Tools?

    1. My mistake, I was not exactly sure whether they were separate or not at the time. It is fixed now.

  3. bot/reviewbot/tools/gotool.py (Diff revision 2)
     
     

    It looks like you're leaning towards making the different uses of go optional, which is good. We should probably default to true for everything except go test, which should default to false (test suites can often be cumbersome and long-running).

    1. That is a good idea, I will make sure to implement it that way.

  4. 
      
jblazusi
jblazusi
david
  1. 
      
  2. bot/reviewbot/tools/gotool.py (Diff revision 3)
     
     

    gofmt still exists, but I believe the modern recommendation is to use go fmt. Can we do that here to be more future-proof?

    1. I believe that this is generally true, however, it is important to note that go fmt runs gofmt -l -w. The -w flag is responsible for overwriting the file, although I do not think that this is an issue, especially since patched files are temporary. But I do think that it is worth noting in case, whomever takes over the project is experiencing issues relating to overwriting a file.

      In short, I have updated the command to use go fmt and this can be found in the new GofmtTool CR.

  3. bot/reviewbot/tools/gotool.py (Diff revision 3)
     
     

    Perhaps "This file contains formatting errors and should be run through go fmt" ?

    Also, shouldn't this be checking the output or return value from the format command? Seems like it just unconditionally adds the comment now (maybe that's still part of the WIP?)

    1. I addressed the message in the new GofmtTool. I originally did not setup conditionals, since I wanted to make sure I was getting the correct output, so this is an artifact from early testing.

  4. bot/reviewbot/tools/gotool.py (Diff revision 3)
     
     
     
     
     
     
     
     
     

    Since we have the output already in a variable, it's kind of silly to write it out to a file and load it back in. We can just do:

    try:
        json_data = json.loads(cleaned_output)
    except Exception as e:
        ...
    
    1. This is definitely a blunder. I was a so caught up in using files from previous tools, that I completely forgot about just passing in the output string.

  5. 
      
jblazusi
jblazusi
ceciliawei
  1. 
      
  2. bot/reviewbot/tools/gotool.py (Diff revision 4)
     
     

    The imports should be in alphabetical order. So import json should be placed before import logging.

    1. Thank you, I will make sure I make this update in previous and future CRs.

  3. 
      
david
  1. 
      
  2. bot/reviewbot/tools/gotool.py (Diff revision 4)
     
     

    Using the string's "split" method isn't portable (for example, windows doesn't use "/" as a path splitter). That said, there's both a portable and easier way to do this:

    package = os.path.dirname(path)
    
    1. I was not aware of this being a portability issue, thank you so much for the advice. I have updated my code to use the os.path module in other areas as well.

  3. bot/reviewbot/tools/gotool.py (Diff revision 4)
     
     

    It looks like we're running this for every single file, which means that if you have multiple changed files in a given package, we'll add the test failures multiple times.

    Instead of overriding handle_file, can we instead override handle_files, and use the file list to build a list of changed packages? We can then run the tests once per package.

    1. Excellent, this is exactly what I had in mind when I was optimizing my FBInfer Tool. However, the advice of building a list of changed packages was a very useful starting point.

  4. 
      
jblazusi
Review request changed

Commit:

-61427f66214d6fa076bdeb38f115e9662266f7dc
+8ab7b4cf5335e42e78ad7e233b5972eeccaa9755

Diff:

Revision 5 (+156)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...