FileReadToEndNotReadable keeps failing

[noah8713]

Fixes:
#25614

Signed-off-by: Aliasgar Ginwala aginwala@ebay.com

[repokitteh-read-only]

Hi @noah8713, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #25641 was opened by noah8713.

see: more, trace.

[ravenblackx]

Note, this fix just makes the test a duplicate of the test above it (with a misleading name and some unrelated actions suggesting there's some coverage that there is not).

[phlax]

@ravenblackx can you advise on the best way forward

(following up on #25614 (comment))

if coverage is dropping then i guess that means another test is required for the uncovered lines

also a bit confused - if it is duplicating above test then i would assume that removing the test should have no affect on coverage

[phlax]

i would assume that removing the test ...

nm, i see - currently the test gives some coverage lines, when "fixed" it becomes a dupe of above and loses that coverage

i guess the question is what is needed to properly cover those lines

[phlax]

/backport

[ravenblackx]

Yeah, I think this is a situation where those lines can't be covered and have the test not break in environments where the file permissions don't behave the desirable way (can't mock it out because the test is explicitly of the non-mock filesystem code). I'm not aware of a more reliable way to cause a 'read' operation to fail, after an 'open' operation succeeded.

It's possible the test could be conditional (check that the permissions preclude reading the file after they've been changed, and if they don't, just log that the test was skipped and don't do the EXPECT), which would provide the coverage in the coverage environment where the test works, and then 'skip' in the environment where the test doesn't work.

A reasonable alternative would be to just delete the problem test, annotate that branch as not reasonable to cover, and explicitly reduce the coverage for that file.

[noah8713]

Yeah, I think this is a situation where those lines can't be covered and have the test not break in environments where the file permissions don't behave the desirable way (can't mock it out because the test is explicitly of the non-mock filesystem code). I'm not aware of a more reliable way to cause a 'read' operation to fail, after an 'open' operation succeeded.

It's possible the test could be conditional (check that the permissions preclude reading the file after they've been changed, and if they don't, just log that the test was skipped and don't do the EXPECT), which would provide the coverage in the coverage environment where the test works, and then 'skip' in the environment where the test doesn't work.

A reasonable alternative would be to just delete the problem test, annotate that branch as not reasonable to cover, and explicitly reduce the coverage for that file.

@ravenblackx @phlax thanks. What is the final suggestion? Can we do incremental patch with alternatives post merging this? Please advice.

[ravenblackx]

It looks like this won't merge as-is because it fails coverage, so you'd have to update the coverage file anyway if you're making this change.

My suggestion would be, if we're going for a quick fix for now and maybe iterating later:

1. Change the test case back to the version that isn't meaningless, then comment it out entirely, with an explanatory comment about how it doesn't work in some envs. (Maybe link to this PR or the issue for more context.)
2. Add "source/common/filesystem/posix:96.5" in /test/per_file_coverage.sh (argh so close to not needing an exception!)

[noah8713]

Thanks @ravenblackx : So coverage failure is due to minimal changes as per logs

Code coverage for source/common/filesystem/posix is lower than limit of 96.6 (96.5)
##[error]Bash exited with code '1'.
Finishing: Run CI script bazel.coverage

So for my knowledge why do we need to touch coverage for minimal changes?

Follow up on your suggestions if I understand correctly:
For 1. I an comment out in this PR TEST_F(FileSystemImplTest, FileReadToEndNotReadable) { and add issue link.
For 2. do you mean for call we need to use this https://github.com/envoyproxy/envoy/blob/main/source/common/filesystem/posix/filesystem_impl.cc#L96 so that for linux it wont be showing not needing exception ? . I was thinking if we comment out we can skip 2 for now too. Just not getting the need for this. Please help clarify. Let me know

[phlax]

Just not getting the need for this. Please help clarify

my understanding re point 1 is yep, that is correct

point 2 is to add an entry for source/common/filesystem/posix in here https://github.com/envoyproxy/envoy/blob/main/test/per_file_coverage.sh - to lower the coverage expectation

[repokitteh-read-only]

CC @envoyproxy/coverage-shephards: FYI only for changes made to (test/per_file_coverage.sh).
envoyproxy/coverage-shephards assignee is @RyanTheOptimist

🐱

Caused by: #25641 was synchronize by noah8713.

see: more, trace.

[noah8713]

Just not getting the need for this. Please help clarify

my understanding re point 1 is yep, that is correct

point 2 is to add an entry for source/common/filesystem/posix in here https://github.com/envoyproxy/envoy/blob/main/test/per_file_coverage.sh - to lower the coverage expectation

Thanks a bunch @phlax and @ravenblackx for suggestions. Addressed comments. PTAL

[noah8713]

@ravenblackx @phlax as per suggestions commenting the code was more work as format checks kept failing. :)

./test/common/filesystem/filesystem_impl_test.cc:112:// #ifndef WIN32
                                                         ^
Checked 4461 file(s) and 121304 comment(s), found 1 error(s).
ERROR: spell check failed. Run 'tools/spelling/check_spelling_pedantic.py fix and/or add new words to tools/spelling/spelling_dictionary.txt'

Anyways adjusted as requested to comment out with original working state and issue link so someone else might not just re change it to failing state . PTAL as most of the checks passed.

[noah8713]

The current failure is due to bash

E: The repository 'https://packages.microsoft.com/ubuntu/20.04/prod focal Release' is no longer signed.
##[error]Bash exited with code '100'.
Finishing: Bash

no formatting other failures @ravenblackx @phlax PTAL

[phlax]

ill hold the 1.25 release ( #25828 ) till this has landed so we can backport it - cc @RyanTheOptimist

[phlax]

backporting this to 1.25 now - apologies i missed doing it before