feat(apiPath): add support for arrays and/or regexp when matching #29

Wolfium · 2015-09-13T03:54:26Z

It include tests for added feature

seriema · 2015-09-13T07:26:58Z

The build-failure seems be because of SauceLabs credentials not working. It got fixed now in #36 so you can update your fork (see instructions). Then try a git push --force to get Github to retrigger the builds and tests.

Btw, a tip for future PR's is to create a new branch on your fork so it's easier for you to keep things updated.

seriema · 2015-09-13T07:34:17Z

karma.conf.js

@@ -24,7 +24,8 @@ module.exports = function(config) {
 			// source files, that you wanna generate coverage for
 			// do not include tests or libraries
 			// (these files will be instrumented by Istanbul)
-			'app/scripts/**/*.js': sourcePreprocessors
+			'app/scripts/**/*.js': sourcePreprocessors,
+			'test/spec/**/*.js': sourcePreprocessors


As you can see on line 25 the comment says not to add the test files. It messes with the test coverage data.

Sorry I didn't pay attention to the comments.

It is an long discussion to include unit tests in coverage or not.
I prefer to include them as they ensure that all lines of test code are executed, I mean, would be possible to get some line of expected assertion never been called and without coverage you never will be able to find out and the will never be evaluated.

Regarding messing values, while it would be true I managed to get a 100% test case coverage which should not affect too much on library values apart from more lines counted for percentage.

I would let them in but let me know what you think and will follow...

Hmm interesting point. It would be really bad to have test code that never runs. Opinions aside, that comment is copied from Karma's Istanbul notes and I can't see test inclusion mentioned in Istanbul's notes on ignoring code that you mention in another comment. It also went to 100% even though Coveralls shows untested lines. So I'm skeptical at the moment =) Either way, that should be a separate PR as it's not related to new functionality.

Regarding ignore directives they are for difficult code paths on the tested code, not tests code necessary, that is the reason you don't see mentions in Istanbul, both practices are related to coverage in any platform and technology.

Tried and can't find where I read about, will give it another try and let you know...

Yes you could read it like that, but since it mostly talks about hard tested code and no example showed test code... well =)

seriema · 2015-09-13T07:43:55Z

It's so awesome of you to help! It would be great if you could add documentation to the readme on how to use the feature.

Wolfium · 2015-09-14T04:14:33Z

Almost done, just need to add documentation on readme and separate test case each as you stated, will let you know when finished tomorrow to be reviewed and merged

Wolfium · 2015-09-14T16:20:40Z

All improvements done.

I reverted the dist files update buy not sure how to rewrite git history to remove the wrong commit.

seriema · 2015-09-14T19:55:08Z

Thanks @Wolfium ! There's a nice answer on StackOverflow that you can follow to squash this into one commit. Check the "Optional - Cleaning commit history" section of the answer. You shouldn't need the git remote add parent part though, as my remote should already be in your list as you did a PR. Type git remote and see if there's something other than "origin". I'd guess you have "origin" and "seriema". In the last step you'll be force pushing it to Github with git push -f, but you can test it out with git push -f --dry-run first to make sure nothing's going to break mid way.

I'm available on Gitter if you want to chat with me while you try it.

seriema · 2015-09-14T19:56:50Z

app/scripts/angular-apimock.js

@@ -206,7 +206,33 @@ angular.module('apiMock', [])
 		}

 		function isApiPath(url) {
-			return url.indexOf(config.apiPath) === 0;
+		return (apiPathMatched(url, config.apiPath) !== undefined);


Indentation please =P

Didn't see it.. 👍

seriema · 2015-09-17T21:19:45Z

Great work so far @Wolfium ! If you could squash your commit I'd be happy to merge it =)

Wolfium · 2015-09-17T22:27:42Z

I didn't forget but not to much time for the squash, will back to this as soon as possible.

It includes documentation updated and tests for added feature Fix #16

Wolfium · 2015-09-23T15:25:37Z

Hi @seriema, it took me a while to get a single commit, but I learn a lot about git. 👍

I hope now it is OK to be merged, I would like to use the new version from official repo.
Let me know.

Regards

seriema · 2015-09-23T15:36:47Z

Wow! Great work @Wolfium !!! Thank you so much for your patience!! =D Yeah, I learned this lesson the hard way when I did the Grunt PR for Twitter Bootstrap. Took me 3 months and then in less than a day they go from asking me to squash, to squashing a copy themselves and close my PR 😆

feat(apiPath): add support for arrays and/or regexp when matching

seriema · 2015-09-23T16:01:50Z

@Wolfium feel free to take a look at #24 or comment on it with some ideas of how you'd like to use it. I'd like to include it before releasing a new version but I won't have much this week because a friend is visiting and it's been planned for months.

Wolfium · 2015-09-23T18:18:08Z

Just from the starting comment about multiple routes this would be related to #28

seriema · 2015-09-23T18:30:09Z

Yepp, I noted that at the bottom of 28 and you can see the reference comment autogenerated by github.

Wolfium · 2015-09-23T18:35:10Z

👍

seriema · 2015-10-05T19:33:49Z

0.3.0 is out! Thanks @Wolfium 👍

Wolfium · 2015-10-05T19:35:19Z

Great, today will update to new version...!!!
Thanks to you

On Mon, Oct 5, 2015 at 4:33 PM John-Philip Johansson <
notifications@github.com> wrote:

0.3.0 is out! Thanks @Wolfium https://github.com/Wolfium [image: 👍]

—
Reply to this email directly or view it on GitHub
#29 (comment)
.

Saludos.
Nestor

seriema reviewed Sep 13, 2015
View reviewed changes

This was referenced Sep 13, 2015

Ability to mock several APIs #18

Closed

SauceLabs failing on PR's #31

Closed

seriema reviewed Sep 14, 2015
View reviewed changes

feat(apiPath): add support for arrays and/or regexp when matching

0a08a7d

It includes documentation updated and tests for added feature Fix #16

seriema added a commit that referenced this pull request Sep 23, 2015

Merge pull request #29 from Wolfium/master

db8f298

feat(apiPath): add support for arrays and/or regexp when matching

seriema merged commit db8f298 into seriema:master Sep 23, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(apiPath): add support for arrays and/or regexp when matching #29

feat(apiPath): add support for arrays and/or regexp when matching #29

Wolfium commented Sep 13, 2015

seriema commented Sep 13, 2015

seriema Sep 13, 2015

Wolfium Sep 14, 2015

seriema Sep 14, 2015

Wolfium Sep 14, 2015

seriema Sep 14, 2015

seriema commented Sep 13, 2015

Wolfium commented Sep 14, 2015

Wolfium commented Sep 14, 2015

seriema commented Sep 14, 2015

seriema Sep 14, 2015

Wolfium Sep 14, 2015

seriema commented Sep 17, 2015

Wolfium commented Sep 17, 2015

Wolfium commented Sep 23, 2015

seriema commented Sep 23, 2015

seriema commented Sep 23, 2015

Wolfium commented Sep 23, 2015

seriema commented Sep 23, 2015

Wolfium commented Sep 23, 2015

seriema commented Oct 5, 2015

Wolfium commented Oct 5, 2015

feat(apiPath): add support for arrays and/or regexp when matching #29

feat(apiPath): add support for arrays and/or regexp when matching #29

Conversation

Wolfium commented Sep 13, 2015

seriema commented Sep 13, 2015

seriema Sep 13, 2015

Choose a reason for hiding this comment

Wolfium Sep 14, 2015

Choose a reason for hiding this comment

seriema Sep 14, 2015

Choose a reason for hiding this comment

Wolfium Sep 14, 2015

Choose a reason for hiding this comment

seriema Sep 14, 2015

Choose a reason for hiding this comment

seriema commented Sep 13, 2015

Wolfium commented Sep 14, 2015

Wolfium commented Sep 14, 2015

seriema commented Sep 14, 2015

seriema Sep 14, 2015

Choose a reason for hiding this comment

Wolfium Sep 14, 2015

Choose a reason for hiding this comment

seriema commented Sep 17, 2015

Wolfium commented Sep 17, 2015

Wolfium commented Sep 23, 2015

seriema commented Sep 23, 2015

seriema commented Sep 23, 2015

Wolfium commented Sep 23, 2015

seriema commented Sep 23, 2015

Wolfium commented Sep 23, 2015

seriema commented Oct 5, 2015

Wolfium commented Oct 5, 2015