rubberduck-vba / Rubberduck

Every programmer needs a rubberduck. COM add-in for the VBA & VB6 IDE (VBE).
https://rubberduckvba.com
GNU General Public License v3.0
1.92k stars 301 forks source link

False Positive Inspection: 'TestMethod' was expected to have less arguments. #5460

Closed Randalliser closed 4 years ago

Randalliser commented 4 years ago

Rubberduck version information Version 2.5.0.5410 OS: Microsoft Windows NT 10.0.18363.0, x64 Host Product: Microsoft Office x64 Host Version: 16.0.12430.20184 Host Executable: EXCEL.EXE

Description The 'Code Inspections' pane reports a False Positive inspection result.

Warning: The annotation 'TestMethod' was expected to have less arguments.

Even if the following annotation is used

'@Ignore SuperfluousAnnotationArgument

To Reproduce Steps to reproduce the behaviour:

  1. create a standard module (.bas)
  2. insert the code (below)
  3. refresh the 'Code Inspections' pane
  4. view the false positive result
  5. open the 'Test Explorer'
  6. the category field on the test correctly shows: 'TheCategory'
  7. back in the 'Code Inspections' pane, select 'Ignore Once'
  8. refresh the 'Code Inspections' pane
  9. view the false positive result

Example Code

'@TestModule
'@Folder("Tests")

Option Explicit
Option Private Module

'@TestMethod("TheCategory")
Private Sub TheTestMethod()                        'TODO Rename test
    On Error GoTo TestFail

    'Arrange:

    'Act:

    'Assert:
    'Assert.Succeed

TestExit:
    'Exit Sub
TestFail:
    'Assert.Fail "Test raised an error: #" & Err.number & " - " & Err.Description
End Sub

Expected behaviour Code inspection pane should not report an exception.

Screenshot Screenshot

Logfile RubberduckLog.txt

Additional context n/a

Vogel612 commented 4 years ago

The root cause here seems to be that optional arguments and nullable value types interact in a way that was not correctly accounted for. As can be observed in the TestMethodAnnotation constructor, we do not explicitly specify a required number of arguments.
In the AnnotationBase constructor this is then passed as an optional parameter defaulting to 0, making all annotations that do not explicitly specify the number of arguments they require assume that they require 0 arguments.

In and of itself that default is sensible, but it's incorrect for @TestMethod. Because the default value of the optional argument is 0, the SuperfluousAnnotationArgumentInspection classifies the annotation as a result here.

I would assume that the problem with the ignore annotation comes from the incorrect assumption in the base inspection code that only processes ignore annotations as attached to a declaration or a line, assuming inspections will only target those specific syntactic elements and not "other annotations" as well.

Additionally this surfaces the problem that @TestMethod only allows a single category to be specified. I'm not sure whether that was intended.

MDoerner commented 4 years ago

This will be fixed after my next PR. It was a misconception on my side when setting up the max allowed arguments for annotations.

retailcoder commented 4 years ago

Side note, "expected to have less arguments" should be "expected to have fewer arguments".