Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ignore/forcefail/forcepass as regexes #75

Closed
M4tteoP opened this issue Sep 14, 2022 · 5 comments · Fixed by #80
Closed

ignore/forcefail/forcepass as regexes #75

M4tteoP opened this issue Sep 14, 2022 · 5 comments · Fixed by #80
Labels
enhancement New feature or request

Comments

@M4tteoP
Copy link
Member

M4tteoP commented Sep 14, 2022

Hello,
I was wondering if it could be helpful to provide to the testoverride lists (ignore, forcefail and forcepass) a regex syntax like the -i/-e commands. The use case comes from a willingness to exclude all the tests of some specific rules (or even a specific file?) and, currently, the only way I see is listing individually all the tests in a quite verbose way.

Before:

...
testoverride:
  ignore:
  '932140-1': 'Currently excluding rule 932140'
  '932140-2': 'Currently excluding rule 932140'
  '932140-4': 'Currently excluding rule 932140'
  '932140-5': 'Currently excluding rule 932140'
  '932140-6': 'Currently excluding rule 932140'
  '932140-7': 'Currently excluding rule 932140'
  '932140-8': 'Currently excluding rule 932140'
  '932140-9': 'Currently excluding rule 932140'
  '932140-10': 'Currently excluding rule 932140'
  '932140-11': 'Currently excluding rule 932140'
  ...

After:

...
testoverride:
  ignore:
  '932140-*': 'Currently excluding rule 932140'

My concern is about the implementation: the check would become not just looking for the a value inside the map (e.g. ignore), but iterating over a list of regex looking for a match. Do you think it's worth the overhead?

Thanks for the feedbacks!

@theseion
Copy link
Collaborator

That's a great idea. Concerning implementation, I don't think performance would suffer much since the amount of data to match is pretty short, as is would the regular expression.

@theseion theseion added the enhancement New feature or request label Sep 17, 2022
@fzipi
Copy link
Member

fzipi commented Sep 21, 2022

It is a good idea, indeed. Let's add it to our backlog.

@M4tteoP
Copy link
Member Author

M4tteoP commented Sep 23, 2022

Hi, I wrote some (most likely dumb) code: Draft PR #80 .
My concerns are about the change of the basic behaviour of simple rules.
A super trivial ignore of a single rule:

  ignore:
  '932140-1': 'Currently excluding rule 932140'

Now that becomes a regex leads to also remove rules like 932140-1, 932140-10, 932140-11.
The rule should now become at least '932140-1$', asserting the position at the end of the line. I feel it is a bit misleading if you face for the first time this functionality.

Thoughts about it? Could it be better to add other lists (something like ignorePattern or ignoreRegex) for this functionality?

...
testoverride:
  ignore:
    '932140-1': 'Currently excluding a specific test'
  ignorePattern:
    '932150-*': 'Currently excluding all the tests of a rule'

@fzipi
Copy link
Member

fzipi commented Sep 23, 2022

Cool, thanks for your contribution!

I think we can simplify this, as we moved test inclusion/exclusion to regexp's. It will make things easier for everyone if we use the same pattern.

That should simplify things a lot. And we only need to document it.

@M4tteoP
Copy link
Member Author

M4tteoP commented Oct 3, 2022

Hi there, I implemented it in a bit better way (at least avoiding regex generation inside loops 😅) and added a bit of documentation about it. I removed the draft status from the PR, please provide me any advice about better implementing it!
Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants