Cleanup user share at end of testAddShareAccepted

[phil-davis]

Description

PR #35666 demonstrates that apps/files_sharing/tests/External/ManagerTest.php testAddShareAccepted is leaving behind a share entry. That is causing the potential for later unit tests to fail.

OCA\Files_Sharing\Tests\Command\CleanupRemoteStoragesTest::testCleanup has been failing intermittently. The unit tests in apps are loaded and executed not necessarily in sctrict alphabetical order. The fail happens when Command/CleanupRemoteStoragesTest happens to run after External/ManagerTest.php. Note: PR #35665 suggests making the unit test loading order deterministic, to help avoid the "random" nature of issues like this in the future.

apps/files_sharing/tests/External/ManagerTest.php testAddShareAccepted was added by PR #35312 which was merged a few weeks ago. The intermittent problem has been around "for a while" - and I guess has been around for a few weeks.

This PR explicitly removes the share created during testAddShareAccepted

Related Issue

#35658

Motivation and Context

Do not leave after-effects at the end of "unit" tests.

Note: lots of these "unit" tests are not really "unit" tests. They are integration tests - they do not mock the known universe, they do real stuff in a real database and have to be careful to clean up the environment so as not to mess up later tests. But that is words, the tests need to happen somewhere.

How Has This Been Tested?

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Database schema changes (next release will require increase of minor version instead of patch)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised:

Open tasks:

• Backport (if applicable set "backport-request" label and remove when the backport was done)

[phil-davis]

We still get out-of-memory PHPunit erros unrelated to this PR.
e.g. https://drone.owncloud.com/owncloud/core/18865/155

PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Runtime:       PHPDBG 7.1.29-1+ubuntu16.04.1+deb.sury.org+1
Configuration: /drone/src/tests/phpunit-autotest.xml

.............................................................   61 / 9677 (  0%)
........................S....................................  122 / 9677 (  1%)
.............................................................  183 / 9677 (  1%)
.............................................................  244 / 9677 (  2%)
.............................................................  305 / 9677 (  3%)
.............................................................  366 / 9677 (  3%)
.............................................................  427 / 9677 (  4%)
.............................................................  488 / 9677 (  5%)
.............................................................  549 / 9677 (  5%)
.............................................................  610 / 9677 (  6%)
.............................................................  671 / 9677 (  6%)
.............................................................  732 / 9677 (  7%)
..............
[PHP Fatal error:  Allowed memory size of 4294967296 bytes exhausted (tried to allocate 65536 bytes) in /drone/src/lib/composer/pear/archive_tar/Archive/Tar.php on line 2250]
[PHP Fatal error:  Allowed memory size of 4294967296 bytes exhausted (tried to allocate 65536 bytes) in /drone/src/lib/private/Shutdown/ShutDownManager.php on line 58] 

I will restart CI to (hopefully) get a good run.

[codecov]

Codecov Report

Merging #35668 into master will decrease coverage by 0.07%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master   #35668      +/-   ##
============================================
- Coverage     64.93%   64.85%   -0.08%     
  Complexity    18777    18777              
============================================
  Files          1222     1222              
  Lines         70901    70901              
  Branches       1289     1289              
============================================
- Hits          46037    45982      -55     
- Misses        24486    24541      +55     
  Partials        378      378

Flag	Coverage Δ	Complexity Δ
#javascript	53.7% <ø> (ø)	0 <ø> (ø)	⬇️
#phpunit	66.13% <ø> (-0.09%)	18777 <ø> (ø)

Impacted Files	Coverage Δ	Complexity Δ
lib/private/DB/PostgreSqlMigrator.php	0% <0%> (-100%)	6% <0%> (ø)
...ilder/ExpressionBuilder/PgSqlExpressionBuilder.php	0% <0%> (-88.89%)	3% <0%> (ø)
lib/private/DB/AdapterPgSql.php	0% <0%> (-85.72%)	2% <0%> (ø)
lib/private/DB/ConnectionFactory.php	66.66% <0%> (-11.6%)	20% <0%> (ø)
lib/private/Repair/Collation.php	82.5% <0%> (-5%)	12% <0%> (ø)
lib/private/DB/MDB2SchemaManager.php	80.7% <0%> (-3.51%)	17% <0%> (ø)
lib/private/DB/Migrator.php	53.01% <0%> (-2.41%)	30% <0%> (ø)
lib/private/DB/Connection.php	64.7% <0%> (-1.48%)	49% <0%> (ø)
lib/private/legacy/db.php	70.51% <0%> (-1.29%)	33% <0%> (ø)
lib/private/DB/MDB2SchemaWriter.php	92.7% <0%> (-1.05%)	34% <0%> (ø)
... and 5 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a316b77...f4dd956. Read the comment docs.

[codecov]

Codecov Report

Merging #35668 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #35668   +/-   ##
=======================================
  Coverage    48.9%    48.9%           
=======================================
  Files         109      109           
  Lines       10569    10569           
  Branches     1289     1289           
=======================================
  Hits         5169     5169           
  Misses       5022     5022           
  Partials      378      378

Flag	Coverage Δ
#javascript	53.7% <ø> (ø)	⬆️
#phpunit	38.29% <ø> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0edcdbc...3fd0cec. Read the comment docs.

[karakayasemi]

LGTM, looks like we are also creating new mount entries in this class and not removing them after test method.
Should we consider to merge all tidy up steps in a tearDown method?

[phil-davis]

I quickly tried making a teardown method. But testRemoveShare did a lot more mocking, and teardown did not work the same for that. But I did not try hard!

Maybe after this fix, somebody could look into refactoring and making a teardown method.

[karakayasemi]

👍

[phil-davis]

Backport stable10 #35670

[phil-davis]

Note: PR #35312 that introduced testAddShareAccepted was only backported to stable10. It is not in release-10.2.1, so this fixing PR only needs to be backported to stable10 (which has been done)

[phil-davis]

No codecov result.
@DeepDiver1975 @micbar @patrickjahns or anyone who can - please merge