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

🐛Binary-Artifacts: skip files that look like tests #1263

Closed
wants to merge 1 commit into from
Closed

🐛Binary-Artifacts: skip files that look like tests #1263

wants to merge 1 commit into from

Conversation

evverx
Copy link
Contributor

@evverx evverx commented Nov 14, 2021

Should address #1256

I'm not sure how to figure out whether files are tests or not
by just looking at their names but it seems almost all the tools
analyzing source code I've seen so far either skip or demote files
containing "test" in their names. Some go even farther and admit that
they can't detect tests and allow users to specify where tests are one
way or another

Should address #1256

I'm not sure how to figure out whether files are tests or not
by just looking at their names but it seems almost all the tools
analyzing source code I've seen so far either skip or demote files
containing "test" in their names. Some go even farther and admit that
they can't detect tests and allow users to specify where tests are
in configs of some kind.
Copy link
Member

@naveensrinivasan naveensrinivasan left a comment

Choose a reason for hiding this comment

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

Thanks!

checks/binary_artifact.go Show resolved Hide resolved
Copy link
Contributor

@laurentsimon laurentsimon left a comment

Choose a reason for hiding this comment

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

We ignore files in testsdata for all checks. There is also a question around whether discarding binaries is a feature for result producers (repo owners) or result consumers (anyone who want more info about a repo before using it). I agree it's useful for repo owners and we can add this as an ignore list option to scorecard (we can expose it in the GitHub action's policy file) - otherwise I'm worried we're going to be asked to add more and more exception to this. For consumers, the best way to solve this is for the community to agree on naming conventions: testdata is already used by certain (several ?) languages.

@evverx
Copy link
Contributor Author

evverx commented Nov 15, 2021

There is also a question around whether discarding binaries is a feature for result producers (repo owners) or result consumers (anyone who want more info about a repo before using it).

Given that consumers usually report "issues" discovered by various code analyzers to maintainers I think they are all involved one way or another.

For consumers, the best way to solve this is for the community to agree on naming conventions: testdata is already used by certain (several ?) languages.

I replied in #1256 (comment)

@evverx evverx mentioned this pull request Nov 17, 2021
@laurentsimon
Copy link
Contributor

is it fair to say this PR may be closed since we've merged #1300?
I'm sure someone else will raise the same issue for tests. Shall we re-visit when this happens and/or when we have a better understanding of what's a test?

cc @david-a-wheeler would love to add where test files are in the yaml config you mentioned during last meeting!

@evverx
Copy link
Contributor Author

evverx commented Dec 3, 2021

is it fair to say this PR may be closed since we've merged #1300?

I don't think it is. There are projects like elfutils that keep ELF files for testing purposes. Plus, basically almost all projects keeping their seed corpora close to their source code are affected I think

@evverx
Copy link
Contributor Author

evverx commented Dec 3, 2021

I think it can be closed because I don't think that just looking for test is enough though. I asked how CodeQL and LGTM do it in #1074 (comment) so in the meantime it would probably make sense to just wait for a reply.

@laurentsimon on an unrelated note I'm not sure who attends those meetings and whether they can affect any roadmaps but if would be great if for example GitHub fixed Dependabot along with their security tab: #1336. I understand that the idea is to probably discuss high-level issues but it would be great if they could also lead to visible improvements in the tools scorecard itself recommends.

@evverx evverx closed this Dec 3, 2021
@laurentsimon
Copy link
Contributor

I think it can be closed because I don't think that just looking for test is enough though. I asked how CodeQL and LGTM do it in #1074 (comment) so in the meantime it would probably make sense to just wait for a reply.

@laurentsimon on an unrelated note I'm not sure who attends those meetings and whether they can affect any roadmaps but if would be great if for example GitHub fixed Dependabot along with their security tab: #1336. I understand that the idea is to probably discuss high-level issues but it would be great if they could also lead to visible improvements in the tools scorecard itself recommends.

let's create an issue to track other tools improvements, thanks for suggesting

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

Successfully merging this pull request may close these issues.

4 participants