Update: added empty error array check for false negative

[anikethsaha]

Prerequisites checklist

• I have read the contributing guidelines.
• The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Currently in test files for rules and internal rules, if a valid test case in added to invalid test case and have errors set to an empty array, it simply passes the test which should be an error as it is false negative.
Added a check for empty-ness of the array and it will throw error if the errors is empty array

Is there anything you'd like reviewers to focus on?

None
not sure whether it is of type Fix or Chore cause I think it is addressing false negative of test cases. let me know 👍

[mdjermanovic]

Interesting, I can confirm that test cases such as this one will indeed pass in the actual version:

invalid: [
    {
        code: "some_valid_code_for_this_rule",
        errors: []
    }
]

I think we should fix this.

not sure whether it is of type Fix or Chore cause I think it is addressing false negative of test cases.

RuleTester is public API, so this would be at least Fix or Update. I believe it doesn't have to be marked as a Breaking change, but would like more opinions.

[mdjermanovic]

If we decide to fix RuleTester to fail on errors: [], maybe we should do the same for errors: 0.

[anikethsaha]

If we decide to fix RuleTester to fail on errors: [], maybe we should do the same for errors: 0.

true, I will add check for this as well.

RuleTester is public API, so this would be at least Fix or Update. I believe it doesn't have to be marked as a Breaking change, but would like more opinions.

I think fix might be better cause of false results .

[kaicataldo]

Agreed that we should fix this. I would also support adding this as a semver-minor bug fix (or in our current major release cycle if this gets approved in time). Can we change the commit message to from Chore: to Update:?

[anikethsaha]

@mdjermanovic I added check for errors: 0. should it be better if the check was < 1 , idk any tests has negative numbers.
@kaicataldo I changed the 1st commit message and PR title to Update type. let me know if anything else is required to change.

[mdjermanovic]

I added check for errors: 0. should it be better if the check was < 1 , idk any tests has negative numbers.

This should be already caught when the tester compares the expected number with the number of actual errors, though it does make sense to check that explicitly. I don't have a strong opinion on this.

[anikethsaha]

This should be already caught when the tester compares the expected number with the number of actual errors, though it does make sense to check that explicitly.

I think we should keep that as the current one cause that will help more than just a message saying it can't be negative.
so, i think its fine to just check for 0

[anikethsaha]

Anything else is needed for this ?

cc @mdjermanovic

[mdjermanovic]

Anything else is needed for this ?

cc @mdjermanovic

The change looks good in general, let's wait to see if it will get accepted before reviewing details.

[kaicataldo]

I think this is a good change, but I do want to note that this is a breaking change (which is particularly concerning right after we just finished a major release). I would support landing this as a semver-minor bug fix, since I do think the current behavior is not intentional.

[kaicataldo]

Sorry for the delay!

[kaicataldo]

@aladdin-add Can you confirm your concerns have been addressed?

[aladdin-add]

sorry for the late, LGTM!

[mdjermanovic]

LGTM, thanks!