Fixed bad base path edge case #3434

loren-osborn · 2016-08-11T00:51:52Z

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.

Nitpick-CI · 2016-08-11T00:52:01Z

src/Codeception/Configuration.php

+                $relPath = substr($path, $rootPathIndex);
+                $pathPrefix = substr($pathPrefix, $rootPathIndex);
+                $basePathArr = explode(DIRECTORY_SEPARATOR, $pathPrefix);
+                while (


Expected 0 spaces after opening bracket; newline found

... because of overly complex while condition. Please make code simplification suggestions. (@Nitpick-CI, Not you personally, I know you're just an automated tool.)

loren-osborn · 2016-08-12T05:54:21Z

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

DavertMik · 2016-08-12T15:23:09Z

Thanks, will be waiting for update!

loren-osborn · 2016-08-12T17:45:38Z

@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.)

Nitpick-CI · 2016-08-14T08:28:16Z

src/Codeception/Configuration.php

+            return $path;
+        }
+        // peel off optional absoluteness prefixes
+        $relPath         = substr($path,              strlen(   $pathAbsPrefix));


Expected 1 space after comma in function call; 14 found

Space after opening parenthesis of function call prohibited

loren-osborn · 2016-08-14T08:32:17Z

@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 · 2016-08-14T12:03:19Z

@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 · 2016-08-14T12:12:51Z

+1 for tests because it is core functionality

loren-osborn · 2016-08-14T14:32:18Z

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 · 2016-08-14T21:17:03Z

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 · 2016-08-14T23:45:08Z

@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 · 2016-08-15T03:06:50Z

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 · 2016-08-15T06:55:35Z

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

Nitpick-CI · 2016-08-15T16:35:46Z

src/Codeception/Configuration.php

+     * @param string $dirSep
+     * @return string
+     */
+    public static function getRelativeDir($path, $projDir, $dirSep =  DIRECTORY_SEPARATOR)


Incorrect spacing between default value and equals sign for argument $dirSep; expected 1 but found 2

loren-osborn · 2016-08-15T16:40:28Z

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

Nitpick-CI · 2016-08-15T17:01:44Z

src/Codeception/Configuration.php

+            return substr($path, strlen($projDir));
+        }
+        // Identify any absoluteness prefix (like '/' in Unix or "C:\\" in Windows)
+        $pathAbsPrefix    = self::getPathAbsolutenessPrefix($path,    $dirSep);


Expected 1 space after comma in function call; 4 found

loren-osborn · 2016-08-15T21:46:59Z

@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.)

@DavertMik

...with unit tests (Thanks for suggestions from @DavertMik and @Naktibalda)

loren-osborn · 2016-08-16T17:43:58Z

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.)

Naktibalda · 2016-08-17T21:22:15Z

src/Codeception/Configuration.php

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


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

Yes... Probably clearer your way.

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

loren-osborn · 2016-08-24T14:29:29Z

@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 · 2016-08-24T14:37:12Z

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

loren-osborn · 2016-08-24T14:49:49Z

Thanks

DavertMik · 2016-08-27T19:52:21Z

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 · 2016-09-04T14:26:32Z

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 · 2016-09-07T21:58:12Z

Looks good. Will be waiting for it

Naktibalda · 2016-09-22T22:11:58Z

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

DavertMik · 2016-09-28T20:28:27Z

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

loren-osborn · 2017-06-01T14:00:28Z

@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.

@DavertMik

...with unit tests (Thanks for suggestions from @DavertMik and @Naktibalda)

Nitpick-CI reviewed Aug 11, 2016
View reviewed changes

loren-osborn force-pushed the losborn_badBasePathFix branch from bd4f62f to 64bec19 Compare August 14, 2016 07:15

Nitpick-CI reviewed Aug 14, 2016
View reviewed changes

Nitpick-CI reviewed Aug 15, 2016
View reviewed changes

Fixed bad base path edge case

13e668d

...with unit tests (Thanks for suggestions from @DavertMik and @Naktibalda)

loren-osborn force-pushed the losborn_badBasePathFix branch from a89e345 to 13e668d Compare August 16, 2016 17:34

Naktibalda reviewed Aug 17, 2016
View reviewed changes

Naktibalda added the Reviewed label Aug 24, 2016

Naktibalda added the Not ready for merging label Sep 13, 2016

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

DavertMik removed the Not ready for merging label Sep 28, 2016

Naktibalda mentioned this pull request Mar 12, 2017

output of codecept_relative_path() incorrect when codeception.yml not in project root #3433

Closed

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

Fixed bad base path edge case (Codeception#3434)

48cf436

...with unit tests (Thanks for suggestions from @DavertMik and @Naktibalda)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed bad base path edge case #3434

Fixed bad base path edge case #3434

loren-osborn commented Aug 11, 2016

Nitpick-CI Aug 11, 2016

loren-osborn Aug 11, 2016

loren-osborn commented Aug 12, 2016

DavertMik commented Aug 12, 2016

loren-osborn commented Aug 12, 2016 •

edited

Loading

Nitpick-CI Aug 14, 2016

loren-osborn commented Aug 14, 2016

DavertMik commented Aug 14, 2016 •

edited

Loading

sergeyklay commented Aug 14, 2016 •

edited

Loading

loren-osborn commented Aug 14, 2016 •

edited

Loading

Naktibalda commented Aug 14, 2016

loren-osborn commented Aug 14, 2016

loren-osborn commented Aug 15, 2016

loren-osborn commented Aug 15, 2016

Nitpick-CI Aug 15, 2016

loren-osborn commented Aug 15, 2016

Nitpick-CI Aug 15, 2016

loren-osborn commented Aug 15, 2016

loren-osborn commented Aug 16, 2016

Naktibalda Aug 17, 2016 •

edited

Loading

loren-osborn Aug 18, 2016

DavertMik Aug 27, 2016

loren-osborn commented Aug 24, 2016

Naktibalda commented Aug 24, 2016

loren-osborn commented Aug 24, 2016

DavertMik commented Aug 27, 2016

loren-osborn commented Sep 4, 2016

DavertMik commented Sep 7, 2016 •

edited

Loading

Naktibalda commented Sep 22, 2016

DavertMik commented Sep 28, 2016

loren-osborn commented Jun 1, 2017

Fixed bad base path edge case #3434

Fixed bad base path edge case #3434

Conversation

loren-osborn commented Aug 11, 2016

Nitpick-CI Aug 11, 2016

Choose a reason for hiding this comment

loren-osborn Aug 11, 2016

Choose a reason for hiding this comment

loren-osborn commented Aug 12, 2016

DavertMik commented Aug 12, 2016

loren-osborn commented Aug 12, 2016 • edited Loading

Nitpick-CI Aug 14, 2016

Choose a reason for hiding this comment

loren-osborn commented Aug 14, 2016

DavertMik commented Aug 14, 2016 • edited Loading

sergeyklay commented Aug 14, 2016 • edited Loading

loren-osborn commented Aug 14, 2016 • edited Loading

Naktibalda commented Aug 14, 2016

loren-osborn commented Aug 14, 2016

loren-osborn commented Aug 15, 2016

loren-osborn commented Aug 15, 2016

Nitpick-CI Aug 15, 2016

Choose a reason for hiding this comment

loren-osborn commented Aug 15, 2016

Nitpick-CI Aug 15, 2016

Choose a reason for hiding this comment

loren-osborn commented Aug 15, 2016

loren-osborn commented Aug 16, 2016

Naktibalda Aug 17, 2016 • edited Loading

Choose a reason for hiding this comment

loren-osborn Aug 18, 2016

Choose a reason for hiding this comment

DavertMik Aug 27, 2016

Choose a reason for hiding this comment

loren-osborn commented Aug 24, 2016

Naktibalda commented Aug 24, 2016

loren-osborn commented Aug 24, 2016

DavertMik commented Aug 27, 2016

loren-osborn commented Sep 4, 2016

DavertMik commented Sep 7, 2016 • edited Loading

Naktibalda commented Sep 22, 2016

DavertMik commented Sep 28, 2016

loren-osborn commented Jun 1, 2017

loren-osborn commented Aug 12, 2016 •

edited

Loading

DavertMik commented Aug 14, 2016 •

edited

Loading

sergeyklay commented Aug 14, 2016 •

edited

Loading

loren-osborn commented Aug 14, 2016 •

edited

Loading

Naktibalda Aug 17, 2016 •

edited

Loading

DavertMik commented Sep 7, 2016 •

edited

Loading