Skip to content

Conversation

@pimyn-girgis
Copy link
Collaborator


Before sending a pull request, please review Contribution Guidelines:
https://github.com/google/syzkaller/blob/master/docs/contributing.md


@pimyn-girgis pimyn-girgis requested a review from a-nogikh January 2, 2026 13:43
@a-nogikh a-nogikh requested a review from dvyukov January 2, 2026 13:47
Copy link
Collaborator

@a-nogikh a-nogikh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me.
@dvyukov wdyt?

@dvyukov
Copy link
Collaborator

dvyukov commented Jan 2, 2026

Why?
It's trivially easy to lose test/linter coverage, but it's very hard to restore it later. If anything I would prefer to go in the opposite direction and fix more warnings and enable more linters/coverage.

@pimyn-girgis
Copy link
Collaborator Author

Why? It's trivially easy to lose test/linter coverage, but it's very hard to restore it later. If anything I would prefer to go in the opposite direction and fix more warnings and enable more linters/coverage.

I think duplication is not something we should be escaping in tests. They tend to be very repetitive by nature. But I see why we would want to keep it... We could increase the threshold (default is 150, and we set it at an aggressive 60), but that would increase it globally, and not just for test files.

@a-nogikh
Copy link
Collaborator

a-nogikh commented Jan 2, 2026

It's not about all linters, but rather about the duplication linter - arguably, duplicating data in tests is not as bad as in the rest of the code.

@dvyukov
Copy link
Collaborator

dvyukov commented Jan 2, 2026

It's not about all linters, but rather about the duplication linter - arguably, duplicating data in tests is not as bad as in the rest of the code.

Do you mean only reasonable duplication, or all possible duplication in all forms and amounts?

For now for 763 tests we have only 19 nolint: dupl directives.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants