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

Fixed bad base path edge case #3434

Merged
merged 1 commit into from Sep 28, 2016

Conversation

Projects
None yet
5 participants
@loren-osborn
Contributor

loren-osborn commented Aug 11, 2016

This is a fix to the issue I brought up in #3433 .

While the code is a bit rough around the edges, it appears to conform to project guidelines, and resolves the issue. Any suggestions to make the code more elegant are welcome.

@loren-osborn

This comment has been minimized.

Show comment
Hide comment
@loren-osborn

loren-osborn Aug 12, 2016

Contributor

Thanks for the feedback... I will revise the code as soon as I'm able.

Contributor

loren-osborn commented Aug 12, 2016

Thanks for the feedback... I will revise the code as soon as I'm able.

@DavertMik

This comment has been minimized.

Show comment
Hide comment
@DavertMik

DavertMik Aug 12, 2016

Member

Thanks, will be waiting for update!

Member

DavertMik commented Aug 12, 2016

Thanks, will be waiting for update!

@loren-osborn

This comment has been minimized.

Show comment
Hide comment
@loren-osborn

loren-osborn Aug 12, 2016

Contributor

@DavertMik, Thanks... I will update this PR as soon as I can find time to update it.

I didn't see a way to PM you, but wanted to know if you could take a peek at an issue I think I found: 2.2...loren-osborn:losborn_configModsLooksLikeABug It shouldn't be hard to write a unit test to catch this issue... (If a enabled module in the module .yml file has options, the Codeception should [incorrectly] try to enable a module with the name of the module's first option, instead of its own name.)

Contributor

loren-osborn commented Aug 12, 2016

@DavertMik, Thanks... I will update this PR as soon as I can find time to update it.

I didn't see a way to PM you, but wanted to know if you could take a peek at an issue I think I found: 2.2...loren-osborn:losborn_configModsLooksLikeABug It shouldn't be hard to write a unit test to catch this issue... (If a enabled module in the module .yml file has options, the Codeception should [incorrectly] try to enable a module with the name of the module's first option, instead of its own name.)

@loren-osborn

This comment has been minimized.

Show comment
Hide comment
@loren-osborn

loren-osborn Aug 14, 2016

Contributor

@DavertMik, let me know if you found this much easier to follow. I prefer descriptive variable and method names to comments, but I included good comments too.

Contributor

loren-osborn commented Aug 14, 2016

@DavertMik, let me know if you found this much easier to follow. I prefer descriptive variable and method names to comments, but I included good comments too.

@DavertMik

This comment has been minimized.

Show comment
Hide comment
@DavertMik

DavertMik Aug 14, 2016

Member

@loren-osborn looks better now, however I still have doubts. I think code still can be simplified but I'm not sure how 😶

Could you add tests to it? They should go to tests/unit/ConfigurationTest.php.

@Naktibalda @sergeyklay what your thoughts of it?

Member

DavertMik commented Aug 14, 2016

@loren-osborn looks better now, however I still have doubts. I think code still can be simplified but I'm not sure how 😶

Could you add tests to it? They should go to tests/unit/ConfigurationTest.php.

@Naktibalda @sergeyklay what your thoughts of it?

@sergeyklay

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Aug 14, 2016

Member

+1 for tests because it is core functionality

Member

sergeyklay commented Aug 14, 2016

+1 for tests because it is core functionality

@loren-osborn

This comment has been minimized.

Show comment
Hide comment
@loren-osborn

loren-osborn Aug 14, 2016

Contributor

I'm certainly down with tests... It's more a matter of knowing what kind of tests, and where to find them. I just found the right place for unit tests for this particular issue, and the issue I found with Codeception\Configuration::modules():

  • This file: Codeception/tests/unit/Codeception/ConfigurationTest.php for unit tests.

@DavertMik, @sergeyklay, where else do you suggest I put tests for these two issues. I currently don't have a setup where I can run the tests (might I suggest a Vagrantfile with a Puppet, Chef or Ansible configuration... A Makefile for running the tests would be welcome too) so I'll have to rely on Travis and the other CI services to test my tests for now.

Contributor

loren-osborn commented Aug 14, 2016

I'm certainly down with tests... It's more a matter of knowing what kind of tests, and where to find them. I just found the right place for unit tests for this particular issue, and the issue I found with Codeception\Configuration::modules():

  • This file: Codeception/tests/unit/Codeception/ConfigurationTest.php for unit tests.

@DavertMik, @sergeyklay, where else do you suggest I put tests for these two issues. I currently don't have a setup where I can run the tests (might I suggest a Vagrantfile with a Puppet, Chef or Ansible configuration... A Makefile for running the tests would be welcome too) so I'll have to rely on Travis and the other CI services to test my tests for now.

@Naktibalda

This comment has been minimized.

Show comment
Hide comment
@Naktibalda

Naktibalda Aug 14, 2016

Member

I found getRelativeDir() very hard to read.

I think that it would be much easier to unit test it, if project dir was passed in as a parameter:
public static function getRelativeDir($path, $projectDir)

function codecept_relative_path($path)
{
    return \Codeception\Configuration::getRelativeDir($path, \Codeception\Configuration::projectDir());
}

All you need to run ConfigurationTest is PHP,
but if you want to run all tests, try to use Docker as documented in https://github.com/Codeception/Codeception/blob/2.2/tests/README.md

Member

Naktibalda commented Aug 14, 2016

I found getRelativeDir() very hard to read.

I think that it would be much easier to unit test it, if project dir was passed in as a parameter:
public static function getRelativeDir($path, $projectDir)

function codecept_relative_path($path)
{
    return \Codeception\Configuration::getRelativeDir($path, \Codeception\Configuration::projectDir());
}

All you need to run ConfigurationTest is PHP,
but if you want to run all tests, try to use Docker as documented in https://github.com/Codeception/Codeception/blob/2.2/tests/README.md

@loren-osborn

This comment has been minimized.

Show comment
Hide comment
@loren-osborn

loren-osborn Aug 14, 2016

Contributor

@Naktibalda, passing the project directory as an argument is a great idea. I was trying to keep the codecept_relative_path() API, but as you pointed out there is no need for that.

As far as a dev environment, I currently have limited storage to keep multiple VMs around, and my current VM is missing a PECL module that composer requires for Codeception. (I don't want to alter one project's dev environment VM for development of another.) I'll see what I can put together if I have the time, but I won't make any promises with my looming deadlines.

Contributor

loren-osborn commented Aug 14, 2016

@Naktibalda, passing the project directory as an argument is a great idea. I was trying to keep the codecept_relative_path() API, but as you pointed out there is no need for that.

As far as a dev environment, I currently have limited storage to keep multiple VMs around, and my current VM is missing a PECL module that composer requires for Codeception. (I don't want to alter one project's dev environment VM for development of another.) I'll see what I can put together if I have the time, but I won't make any promises with my looming deadlines.

@loren-osborn

This comment has been minimized.

Show comment
Hide comment
@loren-osborn

loren-osborn Aug 15, 2016

Contributor

I think I took what @Naktibalda said to heart. I rewrote this so I can inject both projectDir() and DIRECTORY_SEPARATOR so I will now be able to test the Windows path handling without being on a Windown system. I also rewrote the $path and $projDirhandling for clairity, so both are either being manipulated as strings or as arrays, not one of each. I still need to write the actual tests, but that should be straightforward now.

I'm at dinner now, but hope to

Contributor

loren-osborn commented Aug 15, 2016

I think I took what @Naktibalda said to heart. I rewrote this so I can inject both projectDir() and DIRECTORY_SEPARATOR so I will now be able to test the Windows path handling without being on a Windown system. I also rewrote the $path and $projDirhandling for clairity, so both are either being manipulated as strings or as arrays, not one of each. I still need to write the actual tests, but that should be straightforward now.

I'm at dinner now, but hope to

@loren-osborn

This comment has been minimized.

Show comment
Hide comment
@loren-osborn

loren-osborn Aug 15, 2016

Contributor

Status update: Wrote some tests, found some edge casses I missed. I'll address before I push my code.

Contributor

loren-osborn commented Aug 15, 2016

Status update: Wrote some tests, found some edge casses I missed. I'll address before I push my code.

@loren-osborn

This comment has been minimized.

Show comment
Hide comment
@loren-osborn

loren-osborn Aug 15, 2016

Contributor

Enabled test cases... disabled code changes... test suite should FAIL

Contributor

loren-osborn commented Aug 15, 2016

Enabled test cases... disabled code changes... test suite should FAIL

@loren-osborn

This comment has been minimized.

Show comment
Hide comment
@loren-osborn

loren-osborn Aug 15, 2016

Contributor

@DavertMik, @sergeyklay, @Naktibalda, I think the semaphoreci build needs to be restarted...

Once I get a signoff from one of you, I can get started on a git rebase squash of this code. (Of course, I'm still open to suggestions. It's far from the most readable thing I've written.)

Contributor

loren-osborn commented Aug 15, 2016

@DavertMik, @sergeyklay, @Naktibalda, I think the semaphoreci build needs to be restarted...

Once I get a signoff from one of you, I can get started on a git rebase squash of this code. (Of course, I'm still open to suggestions. It's far from the most readable thing I've written.)

Loren Osborn
Fixed bad base path edge case
...with unit tests
(Thanks for suggestions from @DavertMik and @Naktibalda)
@loren-osborn

This comment has been minimized.

Show comment
Hide comment
@loren-osborn

loren-osborn Aug 16, 2016

Contributor

All changed squashed into single commit. Original unsquashed commits are still available here: https://github.com/loren-osborn/Codeception/tree/losborn_badBasePathFix_unsquashed

Thanks for all of your help. Do any of you want any additional revisions before merging this? (i.e. It may be a good idea to build the output array returned by ConfigurationTest::getRelativeDirTestData() to be generated as a permutation of several test conditions, but I'm unsure if that would be more or less readable. Any feedback is appreciated.)

Contributor

loren-osborn commented Aug 16, 2016

All changed squashed into single commit. Original unsquashed commits are still available here: https://github.com/loren-osborn/Codeception/tree/losborn_badBasePathFix_unsquashed

Thanks for all of your help. Do any of you want any additional revisions before merging this? (i.e. It may be a good idea to build the output array returned by ConfigurationTest::getRelativeDirTestData() to be generated as a permutation of several test conditions, but I'm unsure if that would be more or less readable. Any feedback is appreciated.)

public static function getRelativeDir($path, $projDir, $dirSep = DIRECTORY_SEPARATOR)
{
// ensure $projDir ends with a trailing $dirSep
$projDir = preg_replace('/'.preg_quote($dirSep, '/').'*$/', $dirSep, $projDir);

This comment has been minimized.

@Naktibalda

Naktibalda Aug 17, 2016

Member

Not an issue, but my preferred way is $projDir = rtrim($projDir, $dirSep) . $dirSep;

@Naktibalda

Naktibalda Aug 17, 2016

Member

Not an issue, but my preferred way is $projDir = rtrim($projDir, $dirSep) . $dirSep;

This comment has been minimized.

@loren-osborn

loren-osborn Aug 18, 2016

Contributor

Yes... Probably clearer your way.

@loren-osborn

loren-osborn Aug 18, 2016

Contributor

Yes... Probably clearer your way.

This comment has been minimized.

@DavertMik

DavertMik Aug 27, 2016

Member

Yeah, don't use regex here. rtrim is much clearer

@DavertMik

DavertMik Aug 27, 2016

Member

Yeah, don't use regex here. rtrim is much clearer

}
// peel off optional absoluteness prefixes and convert
// $path and $projDir to an subdirectory path array
$relPathParts = array_filter(explode($dirSep, substr($path, strlen($pathAbsPrefix['wholePrefix']))), 'strlen');

This comment has been minimized.

@Naktibalda

Naktibalda Aug 17, 2016

Member

'strlen' is not necessary, array_filter removes empty strings when callback is not provided.

@Naktibalda

Naktibalda Aug 17, 2016

Member

'strlen' is not necessary, array_filter removes empty strings when callback is not provided.

This comment has been minimized.

@loren-osborn

loren-osborn Aug 18, 2016

Contributor

Agree... Was trying to make the code more clear... I probably failed. ;-)

@loren-osborn

loren-osborn Aug 18, 2016

Contributor

Agree... Was trying to make the code more clear... I probably failed. ;-)

This comment has been minimized.

@loren-osborn

loren-osborn Aug 18, 2016

Contributor

I take back my previous comment... Without a callback, array_filter is supposed to remove any element that evaluates to FALSE... I believe this includes some non-empty strings, including "0"... Looks like I should add some dies named 0 to my data provider test cases.

@loren-osborn

loren-osborn Aug 18, 2016

Contributor

I take back my previous comment... Without a callback, array_filter is supposed to remove any element that evaluates to FALSE... I believe this includes some non-empty strings, including "0"... Looks like I should add some dies named 0 to my data provider test cases.

This comment has been minimized.

@Naktibalda

Naktibalda Aug 18, 2016

Member

then using strlen is fine.

@Naktibalda

Naktibalda Aug 18, 2016

Member

then using strlen is fine.

['\\my\\proj\\path\\some\\dir\\in\\my\\proj\\', '\\my\\proj\\path\\foobar\\', '\\', '..\\some\\dir\\in\\my\\proj\\'],
// Device letter (both)... path absoluteness mismatch
['C:\\my\\proj\\path\\some\\file\\in\\my\\proj.txt', 'C:my\\proj\\path\\', '\\', '\\my\\proj\\path\\some\\file\\in\\my\\proj.txt'],
['d:my\\proj\\path\\some\\file\\in\\my\\proj.txt', 'd:\\my\\proj\\path\\', '\\', 'my\\proj\\path\\some\\file\\in\\my\\proj.txt'],

This comment has been minimized.

@Naktibalda

Naktibalda Aug 17, 2016

Member

Is d:my a valid path on Windows?

@Naktibalda

Naktibalda Aug 17, 2016

Member

Is d:my a valid path on Windows?

This comment has been minimized.

@loren-osborn

loren-osborn Aug 18, 2016

Contributor

To my knowledge it is... It certainly was in the MSDOS days... Each drive letter keeps its own CWD

@loren-osborn

loren-osborn Aug 18, 2016

Contributor

To my knowledge it is... It certainly was in the MSDOS days... Each drive letter keeps its own CWD

return [
// Unix style paths:
// projectDir() with & without trailing directory seperator: actual subdir
['/my/proj/path/some/file/in/my/proj.txt', '/my/proj/path/', '/', 'some/file/in/my/proj.txt'],

This comment has been minimized.

@Naktibalda

Naktibalda Aug 17, 2016

Member

The size of this data provider is impressive, so it must be good.

@Naktibalda

Naktibalda Aug 17, 2016

Member

The size of this data provider is impressive, so it must be good.

This comment has been minimized.

@loren-osborn

loren-osborn Aug 18, 2016

Contributor

I believe you were being sarcastic, but I disagree (so actually agreeing)... I think shorter is clearer... Just not sure how to pair this down better

@loren-osborn

loren-osborn Aug 18, 2016

Contributor

I believe you were being sarcastic, but I disagree (so actually agreeing)... I think shorter is clearer... Just not sure how to pair this down better

This comment has been minimized.

@loren-osborn

loren-osborn Aug 18, 2016

Contributor

... Each device has its own CWD on a per-shell basis, I believe

@loren-osborn

loren-osborn Aug 18, 2016

Contributor

... Each device has its own CWD on a per-shell basis, I believe

This comment has been minimized.

@DavertMik

DavertMik Aug 27, 2016

Member

Yes, the list is impressive. And that's really good

@DavertMik

DavertMik Aug 27, 2016

Member

Yes, the list is impressive. And that's really good

@loren-osborn

This comment has been minimized.

Show comment
Hide comment
@loren-osborn

loren-osborn Aug 24, 2016

Contributor

@DavertMik, @sergeyklay, @Naktibalda, Status update:

  • I have confirmed that D:Foo is a perfectly valid Windows path (referring to file or directory Foo in this shell's current working directory for device D:)
  • While I'm open to some tweaking, I think this patch is basically done and ready to merge.
  • Welcome additions include:
    • Additional test cases with 0 as one of the directories in paths
    • Easier to read generation of test cases instead of an confusing exhaustive list
  • I can get to these when I have time, but consider them low priority.
Contributor

loren-osborn commented Aug 24, 2016

@DavertMik, @sergeyklay, @Naktibalda, Status update:

  • I have confirmed that D:Foo is a perfectly valid Windows path (referring to file or directory Foo in this shell's current working directory for device D:)
  • While I'm open to some tweaking, I think this patch is basically done and ready to merge.
  • Welcome additions include:
    • Additional test cases with 0 as one of the directories in paths
    • Easier to read generation of test cases instead of an confusing exhaustive list
  • I can get to these when I have time, but consider them low priority.
@Naktibalda

This comment has been minimized.

Show comment
Hide comment
@Naktibalda

Naktibalda Aug 24, 2016

Member

Your PR looks good to me, I am waiting for somebody else to review the code before merging it.

Member

Naktibalda commented Aug 24, 2016

Your PR looks good to me, I am waiting for somebody else to review the code before merging it.

@Naktibalda Naktibalda added the Reviewed label Aug 24, 2016

@loren-osborn

This comment has been minimized.

Show comment
Hide comment
@loren-osborn

loren-osborn Aug 24, 2016

Contributor

Thanks

Contributor

loren-osborn commented Aug 24, 2016

Thanks

@DavertMik

This comment has been minimized.

Show comment
Hide comment
@DavertMik

DavertMik Aug 27, 2016

Member

Thanks. I think you did a great job and this PR looks almost perfect to me. Especially tests look great.
However, now I have a feeling that isWindows, getPathAbsolutenessPrefix, and other added methods may not belong to Configuration class. They are not about Configuration, right? That's why I'd suggest to move the path resolving logic to a utility class, probably Codeception\Util\PathResolver and make Configuration only call them to get relative path.

Sorry for this additional request but that's how we can maintain the clean and sane codebase.

Member

DavertMik commented Aug 27, 2016

Thanks. I think you did a great job and this PR looks almost perfect to me. Especially tests look great.
However, now I have a feeling that isWindows, getPathAbsolutenessPrefix, and other added methods may not belong to Configuration class. They are not about Configuration, right? That's why I'd suggest to move the path resolving logic to a utility class, probably Codeception\Util\PathResolver and make Configuration only call them to get relative path.

Sorry for this additional request but that's how we can maintain the clean and sane codebase.

@loren-osborn

This comment has been minimized.

Show comment
Hide comment
@loren-osborn

loren-osborn Sep 4, 2016

Contributor

This seems like a very reasonable request , though I'm leaning toward Codeception\Util\PathHandler unless the other already exists. I don't expect address this this week, but will try to get to it shortly. @DavertMik, if you can suggest a more readable way to generate the given test cases, I would be grateful.

Contributor

loren-osborn commented Sep 4, 2016

This seems like a very reasonable request , though I'm leaning toward Codeception\Util\PathHandler unless the other already exists. I don't expect address this this week, but will try to get to it shortly. @DavertMik, if you can suggest a more readable way to generate the given test cases, I would be grateful.

@DavertMik

This comment has been minimized.

Show comment
Hide comment
@DavertMik

DavertMik Sep 7, 2016

Member

Looks good. Will be waiting for it

Member

DavertMik commented Sep 7, 2016

Looks good. Will be waiting for it

@Naktibalda

This comment has been minimized.

Show comment
Hide comment
@Naktibalda

Naktibalda Sep 22, 2016

Member

@loren-osborn you are about to miss a release.

Member

Naktibalda commented Sep 22, 2016

@loren-osborn you are about to miss a release.

@DavertMik DavertMik merged commit b71a2dd into Codeception:2.2 Sep 28, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
semaphoreci The build passed on Semaphore.
Details
@DavertMik

This comment has been minimized.

Show comment
Hide comment
@DavertMik

DavertMik Sep 28, 2016

Member

Thanks @loren-osborn
We will extract class by ourselves and your fix is about to land in next release

Member

DavertMik commented Sep 28, 2016

Thanks @loren-osborn
We will extract class by ourselves and your fix is about to land in next release

@loren-osborn

This comment has been minimized.

Show comment
Hide comment
@loren-osborn

loren-osborn Jun 1, 2017

Contributor

@DavertMik, @Naktibalda thanks for picking up the slack on this. My last comment in this PR was actually in the few days between when I was laid off from my last employer and when my daughter was born, so you can imagine I was a bit distracted. I ended up being out of work about 6 months and didn't realize you had actually merged this. Thank you for picking up the slack in my absence.

Contributor

loren-osborn commented Jun 1, 2017

@DavertMik, @Naktibalda thanks for picking up the slack on this. My last comment in this PR was actually in the few days between when I was laid off from my last employer and when my daughter was born, so you can imagine I was a bit distracted. I ended up being out of work about 6 months and didn't realize you had actually merged this. Thank you for picking up the slack in my absence.

chris1312 added a commit to chris1312/Codeception that referenced this pull request Jun 16, 2017

Fixed bad base path edge case (#3434)
...with unit tests
(Thanks for suggestions from @DavertMik and @Naktibalda)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment