checkstyle / patch-filters

Suppression Filter for Checkstyle that is based on patch file
GNU Lesser General Public License v2.1
4 stars 6 forks source link

Issue #113: resolves audit event matching on windows #361

Closed rnveach closed 1 year ago

rnveach commented 1 year ago

113

Details of issue at https://github.com/checkstyle/patch-filters/issues/113#issuecomment-1312808670 .

This does NOT resolve the issue, we need a windows CI to confirm.

rnveach commented 1 year ago

https://github.com/checkstyle/patch-filters/actions/runs/3464243982/jobs/5785533873#step:5:57 Linux run is good.

rnveach commented 1 year ago

possible to cover this code changes with test ?

I assume this means to provide a similar test case from Windows to run on Linux and show the same behavior. We are already showing it works by having a Windows self-run and a Linux self-run.

top level tests that takes patch, file, config and run validation.

I don't think we can do it this way since it needs to support Linux and Windows even in their natural environments.

Patch file, even for Windows, is always using / slashes. So Linux and Windows have the same file contents (minus line endings).

All files on Windows is always \r\n but gitattributes can ensure we can give Linux a file like this too.

The issue with running normal validation for testing something like this is File.separatorChar. I am not aware of anyway to override it to provide different values on different OSes. Linux will always be / and Windows will always be \, so there can be no opposite testing. Additionally, for this to really be tested event.getFileName always has to return \ path delimiters on the opposite OS, as same OS is already testing slashes in using their own environment values. Through natural means, each OS will only use their own slashes. The only way to atleast attempt to test this is to invoke JavaPatchFilterElement directly. This is because AuditEvents are controlled by the main repo, and Checker only takes File class where \ on Linux will not work and only works with / otherwise we will get File Not Found exceptions.

Example:

checkstyle$ vi .ci/common.sh ; # opens correct file
checkstyle$ vi .ci\common.sh ; # opens new file ".cicommon.sh"
checkstyle$ vi .ci\\common.sh ; # opens new file ".ci\common.sh"

Let me know if I got anything wrong as I don't use Linux a lot.

TLDR: Direct test is only option I am aware of, and it won't be able to simiulate other File.separatorChar values.

We can set some system properties during test execution mimic windows.

I don't think I followed this. What system properties do you refer to?

rnveach commented 1 year ago

Is it possible to cover this code changes with test ?

I think I just noticed the double postage.

I was assuming we were talking about the \ / / characters. If it is just the endsWith then I can add a test to show my concerns. Everything is already showing with the suggested match.

I am attempting this now.

Edit: re-reading rest this is not what was said and I am just confusing myself more.

romani commented 1 year ago

https://ci.appveyor.com/project/Checkstyle/patch-filters

romani commented 1 year ago

@rnveach , we can merge this PR and release to let it be tested in real life in eclipse-cs. test can be sent in next PR. Should we merge and release ?

rnveach commented 1 year ago

I'm good with a merge and release. I'm busy with work more this week and will need additional time to finish building some tests.

romani commented 1 year ago

Please rebase PR

rnveach commented 1 year ago

Done.

romani commented 1 year ago

appveyor project is remove, as we use github actions