Fix ERROR M310 when directory name starts with .

[jeromecoutant]

Fix #110

See #110 (comment)

@thorstendb-ARM

[github-actions]

Unit Test Results

  6 files  12 suites   2s ⏱️
17 tests 17 ✔️ 0 💤 0 ❌
51 runs  51 ✔️ 0 💤 0 ❌

Results for commit 21f4240.

♻️ This comment has been updated with latest results.

[jeromecoutant]

Merging [#137] will decrease coverage by 13.87%.

I think there is an issue in Codecov tool....

[JonatanAntoni]

I think there is an issue in Codecov tool....

Yes, the CodeCoverage seems to be not properly configured. We need to check.

Can we add some tests for this function?

• Reproduce the original issue.
• Verify the fix does the job.

[jeromecoutant]

Can we add some tests for this function?

I think it's done :-)

Reproduce the original issue.

With only the commit with the new test:

$ cmake --build . --target packchk --target PackChkIntegTests

$ ctest -C Debug -R PackChkIntegTests

Test project C:/github/OpenCMSISPack/devtools/build
    Start 13: PackChkIntegTests
1/1 Test #13: PackChkIntegTests ................***Failed    1.27 sec

[  FAILED  ] PackChkIntegTests.CheckPackWithDot (7 ms)

Verify the fix does the job.

With the patch:

$ cmake --build . --target packchk --target PackChkIntegTests

$ ctest -C Debug -R PackChkIntegTests
1/1 Test #13: PackChkIntegTests ................   Passed    1.45 sec

[JonatanAntoni]

Looks promising. I think we could add a unit test in addition to the integration test.
I am not yet convinced that this routine handles all situations, properly.

[JonatanAntoni]

The coverage reports turns out that the code range under discussion is not properly hit by the provided tests.

[JonatanAntoni]

What are we trying to test here? Filenames starting with a . should be legal. At least on unix-like systems such files/directories are considered "hidden".
Didn't we discuss (and test) for names starting with ./? I.e., the unix-like shortcut for "current working directory"?

[fred-r]

Yes, the intention is to support these "hidden" elements.
At least ".directory" should be accepted as we plan to use it to collect all the files an end-user does not really need to "see".

[JonatanAntoni]

Okay, so this test actually does not verify the removal of ./ prefixes, intentionally. So we definitively need to add some more tests, probably unit tests, to really cover the implementation.

[jeromecoutant]

So we definitively need to add some more tests, probably unit tests, to really cover the implementation

Any update ?
Thx

[fred-r]

What can we do about the codecov issue blocking the merge ?

[jeromecoutant]

@JonatanAntoni any update ?
Thx

[JonatanAntoni]

@jeromecoutant,

We need to provide more tests to be sure the function does what is expected.
Please rebase your patch and add some tests which cover the code under modification.

Cheers,
Jonatan

[jeromecoutant]

Please rebase your patch and add some tests which cover the code under modification.

Done

[JonatanAntoni]

@jeromecoutant, after merging #262 please consider rebasing this PR to keep contributing your integration test. Actually, we should already have everything covered with the unit tests but the additional integ test would not harm.

[jeromecoutant]

@JonatanAntoni ?

[codecov]

Codecov Report

Merging #137 (21f4240) into main (a484c1f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #137   +/-   ##
=======================================
  Coverage   66.60%   66.60%           
=======================================
  Files          24       24           
  Lines        4734     4734           
=======================================
  Hits         3153     3153           
  Misses       1581     1581           

Flag	Coverage Δ
packchk-cov	47.75% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.