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

[WIP] unit testing of module ID in internal getPath() call (1.3.x) #39

Closed

Conversation

brodybits
Copy link

@brodybits brodybits commented Aug 10, 2018

From review of PR #38 for 1.3.1 patch release on 1.3.x branch we discovered a rare issue where the code would use the wrong module ID in case of a URL like https://scm.git.service.io/user/my-repo.git.

There was also a question if the changes in PR #38 may lead to a rare issue in case of a URL like git://scm.git.service.io/user/my-repo.git.

This WIP PR includes unit testing to check the results of the internal getPath() call on some URL variations, fails due to some module ID inconsistencies in case of the following URLs (may have missed a few):

  • https://scm.git.service.io/user/my-repo.git (XXX TODO try these with plain http)
  • git://scm.git.service.io/user/my-repo.git
  • git+http://scm.git.service.io/user/my-repo.git
  • git+https://scm.git.service.io/user/my-repo.git
  • https://scm.service.io/user/my-repo-other-url where npm indicates that it installed my-repo@2.0.0
  • file://my/path/to/my-repo
  • /my/path/to/my-repo (XXX TODO test path starting with . & ..)
  • https://scm.service.io/user/my-repo#old-tag
  • git://scm.service.io/user/my-repo#old-tag
  • git+http://scm.service.io/user/my-repo#old-tag
  • git+https://scm.service.io/user/my-repo#old-tag
  • git+http://scm.service.io/user/my-repo.git (solved by changes proposed in PR GH-37 patch release updates for 1.3.1 #38)
  • git+https://scm.service.io/user/my-repo.git (solved by changes proposed in PR GH-37 patch release updates for 1.3.1 #38)
  • git+http://scm.service.io/user/my-repo.git#old-tag (solved by changes proposed in PR GH-37 patch release updates for 1.3.1 #38)
  • git+https://scm.service.io/user/my-repo.git#old-tag (solved by changes proposed in PR GH-37 patch release updates for 1.3.1 #38)
  • https://scm.git.service.io/user/my-repo.git#old-tag
  • git://scm.git.service.io/user/my-repo.git#old-tag
  • git+http://scm.git.service.io/user/my-repo.git#old-tag
  • git+https://scm.git.service.io/user/my-repo.git#old-tag

The exact same Most of the failures show up on 1.3.x with or without the changes proposed in PR #38.

Note that it is not possible to run these exact unit tests on master due to some changes in the internal implementation. I think it is necessary to add similar unit tests to master to ensure that this kind of issue is resolved and does not come back in the future.

Some failures due to issues with internal trimID() function

I am suspicious that these errors may lead to actual failures in
some very rare cases.

This is tested on 1.3.x only. Version in master branch (2.0.0-dev)
has undergone some massive changes and may not suffer from this
kind of an issue. I think more testing is needed there to be sure.
Christopher J. Brody added 2 commits August 10, 2018 16:20
@brodybits
Copy link
Author

brodybits commented Aug 10, 2018

Quick analysis for anyone interested:

The module ID used in the internal getPath() call is mangled differently in 1.3.x the update from GH-12 (PR #12) as proposed in PR #38:

  • git+http://scm.git.service.io/user/my-repo.git
  • git+https://scm.git.service.io/user/my-repo.git
  • git+http://scm.git.service.io/user/my-repo.git#old-tag
  • git+https://scm.git.service.io/user/my-repo.git#old-tag

In case the change in GH-12 is applied then target module ID=scm would be used in the internal getPath() call.

Yes this should (hopefully) be gone in the master branch, would like to cover in testing if possible.

P.S. Here is a quick fix patch:

diff --git a/index.js b/index.js
index 8bb6326..7122e45 100644
--- a/index.js
+++ b/index.js
@@ -157,12 +157,16 @@ function trimID (target) {
         target = gitInfo.project;
     } else if (isUrl(target) || isGitUrl(target)) {
         // strip away .git and everything that follows
-        var strippedTarget = target.split('.git');
+        // var strippedTarget = target.split('.git');
+        // var strippedTarget = [ target.replace(/\.git(#[\.0-9]*)?$/, '') ];
+        var strippedTarget = [ target ];
         var re = /.*\/(.*)/;
         // Grabs everything in url after last `/`
         parts = strippedTarget[0].match(re);
 
         target = parts[1];
+        // target = target.replace(/\.git(#[\.0-9]*)?$/, '');
+        target = target.replace(/\.git(#.*)?$/, '');
     }
 
     // If local path exists, try to get plugin id from package.json or set target to final directory

@janpio janpio changed the title WIP unit testing of module ID in internal getPath() call (1.3.x) [WIP] unit testing of module ID in internal getPath() call (1.3.x) Aug 14, 2018
@brodybits
Copy link
Author

As discussed in #40 we think this is not needed in the 1.3.x branch. I would like to test some of these cases on the master branch at some point, which would have to be done differently due to the change in internal implementation.

@brodybits brodybits closed this Aug 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant