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

plugin tests need improvements #543

Open
vivien opened this issue Jan 31, 2015 · 4 comments
Open

plugin tests need improvements #543

vivien opened this issue Jan 31, 2015 · 4 comments
Labels

Comments

@vivien
Copy link
Member

vivien commented Jan 31, 2015

plugin tests don't support multiple matches

The following line:

Hey! Check this https://www.youtube.com/watch?v=HO1OV5B_JDw and https://www.youtube.com/watch?v=nVjsGKrE6E8 :-)

will embbed 2 Youtube videos. However, adding this line to the test cases will fail.

Plugin tests don't check output

Tests must be extended to also check the expected output. If you're regex has multiple matches and you made the mistake to return the bad index (let's say undefined), the test will still pass.

For instance, try the following patch:

diff --git a/js/plugins.js b/js/plugins.js
index fd9bf51..d364a9b 100644
--- a/js/plugins.js
+++ b/js/plugins.js
@@ -197,7 +197,7 @@ plugins.factory('userPlugins', function() {

         // iterate over all matches
         while (match !== null){
-            var token = match[1];
+            var token = match[0];
             var embedurl = "https://www.youtube.com/embed/" + token + "?html5=1&iv_load_policy=3&modestbranding=1&rel=0&showinfo=0";
             content.push('<iframe width="560" height="315" src="'+ embedurl + '" frameborder="0" allowfullscreen frameborder="0"></iframe>');
             // next match

Note on regexp

Plugins must be written in the way that they support multiple contents per line.
We might want to impose the following regexp syntax: /https?:\/\/(www\.)?foobar?/g
please note the global g flag and no line delimiters (^ or $).

Also, the plugin code must be written like while (match) { content.push(); } return content; (see the youtube plugin or the Vine plugin).
Maybe this peace of code can be factorized which would allow to only provide the regexp portion to compile and the expected output. For instance:

var vinePlugin = new Plugin("Vine", "vine.co/v/([a-zA-Z0-9]+)(/.*)?", function(matches) {
    var id = matches[0];
    var embedurl = "https://vine.co/v/" + id + "/embed/simple?audio=1";
    return '<iframe class="vine-embed" src="' + embedurl + '" width="600" height="600" frameborder="0"></iframe><script async src="//platform.vine.co/static/scripts/embed.js" charset="utf-8"></script>';
});
@lorenzhs
Copy link
Member

That second thing is exactly why I created urlPlugin, to simplify that! You're right about the first one as well, we need to improve our tests.

@lorenzhs lorenzhs added the plugin label Feb 2, 2015
@lorenzhs
Copy link
Member

lorenzhs commented Feb 8, 2015

I've addressed your second part, making urlPlugin more convenient, in #547

@vivien
Copy link
Member Author

vivien commented Feb 8, 2015

Looks good for me!

@lorenzhs
Copy link
Member

lorenzhs commented Feb 8, 2015

👍 cool!

vivien added a commit to vivien/glowing-bear that referenced this issue Feb 8, 2015
Extend UrlPlugin to take an optional regexp pattern, and pass the
matches array to the plugin callback.

This allows to factorize the case insensitivity ('i' flag), and get rid
of redundant match check in plugins:

    var regexp = /.../;
    var match = url.match(regexp);
    if (match) {
        ...
    }

Refs glowing-bear#543 (part 3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants