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

Don't automatically rewind DirectoryScanner file handle #768

Merged
merged 2 commits into from
Jan 10, 2019

Conversation

smgallo
Copy link
Contributor

@smgallo smgallo commented Jan 4, 2019

Description

When running the DirectoryScanner ETL DataEndpoint in docker using the latest overlay2 storage driver and PHP 5.4, the DirectoryScannerTest::testPatternFilters() automated tests would fail because an incorrect number of files (exactly 2 times the expected number) was returned from a directory scan. When using the aufs driver (as shippable.com does) this did not manifest.

The error occurs when the CallbackFilterIterator::rewind() method is called twice in a row. The set of records returned by the iterator is duplicated. Since we iterate over the set using foreach and a rewind() is called under the hood by PHP it is not necessary to explicitly call it ourselves.

Note that this bug does not manifest in PHP7 running in docker.

Motivation and Context

Rescue developers from insanity wondering why tests that have been working for years are now failing.

Tests performed

Manual docker testing and automated testing in docker.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project as found in the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@smgallo smgallo added the bug Bugfixes label Jan 4, 2019
@smgallo smgallo added this to the 8.1.0 milestone Jan 4, 2019
ryanrath
ryanrath previously approved these changes Jan 4, 2019
@smgallo smgallo changed the base branch from xdmod8.1 to xdmod8.0 January 6, 2019 01:21
@smgallo smgallo changed the base branch from xdmod8.0 to xdmod8.1 January 6, 2019 01:22
@smgallo smgallo changed the base branch from xdmod8.1 to xdmod8.0 January 6, 2019 02:19
@plessbd
Copy link
Contributor

plessbd commented Jan 6, 2019

something seems a little wonky here, we shouldnt upgrade the 8.0 branch to the 8.0 docker either...

@smgallo smgallo changed the base branch from xdmod8.0 to xdmod8.1 January 6, 2019 14:01
@smgallo
Copy link
Contributor Author

smgallo commented Jan 6, 2019

Even if I run the integration tests locally against the v2 docker using the unmodified 8.1 branch I get failures. Either I have something screwed up or something else is wonky.

@smgallo smgallo merged commit 24bd7d9 into xdmod8.1 Jan 10, 2019
@smgallo smgallo deleted the fix-failing-tests-in-docker branch January 10, 2019 14:01
@smgallo smgallo added the Category:ETL Extract Transform Load label Mar 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bugfixes Category:ETL Extract Transform Load
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants