Removes escaping of double-quotes around string value

[lorengordon]

What did you implement:

When using the new string syntax for a variable, a double-quoted string would be rendered with the double-quotes escaped around the value.

How can we verify it:

serverless.yml:

service: hello

provider:
  name: aws
  runtime: nodejs6.10
  environment:
    STRING_DOUBLEQUOTE: ${env:STRING_DOUBLEQUOTE, "foo"}
    STRING_SINGLEQUOTE: ${env:STRING_SINGLEQUOTE, 'foo'}

functions:
  hello:
    handler: handler.hello

Rendered cfn template without the patch:

        "Environment": {
          "Variables": {
            "STRING_DOUBLEQUOTE": "\"foo\"",
            "STRING_SINGLEQUOTE": "foo"
          }
        }

Rendered cfn template with the patch:

        "Environment": {
          "Variables": {
            "STRING_DOUBLEQUOTE": "foo",
            "STRING_SINGLEQUOTE": "foo"
          }
        }

Todos:

• Write tests
• Write documentation
• Fix linting errors
• Make sure code coverage hasn't dropped
• Provide verification config / commands / resources
• Enable "Allow edits from maintainers" for this PR
• Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

[lorengordon]

Seeing as the tests pass with and without this patch, I'm pretty sure the tests are not really setting up the inputs and outputs correctly. The tests look reasonable and I'm not seeing what's wrong with them, but if someone wants to point me at the problem, I can patch those too.

[lorengordon]

Oh I see it. Umm, I think a lot of the variable tests may not be properly setting up the inputs and outputs. The tests are actually mocking the return value of populateVariable, which is the value that is being tested.

In effect, we're declaring that populateVariable returns the sky is green and then testing that the returned value of the mocked function is the sky is green. Well, yeah, the logic is circular.

Should probably not be mocking (stubbing?) populateVariable, at least not in the overwrite tests.

[lorengordon]

Removed the stub for populateVariable, and now I'm seeing that the overwrite stub is also causing the same behavior. That's not right either, we aren't really exercising the overwrite method.

But if I remove the overwrite stub, then we get the error: TypeError: Cannot read property 'stage' of undefined. Seems to indicate the test setup is not quite right.

[lorengordon]

Ok, this test exposes the problem on the master branch:

    it('should allow a double-quoted string if overwrite syntax provided', () => {
      const serverless = new Serverless();
      const property = 'my stage is ${opt:stage, "prod"}';
      serverless.variables.options = {
        stage: undefined,
      };

      serverless.variables.loadVariableSyntax();

      return serverless.variables.populateProperty(property).then(newProperty => {
        expect(newProperty).to.equal('my stage is prod');

        return BbPromise.resolve();
      });
    });

[lorengordon]

There we go. Now the string syntax tests are calling the functions being tested.

[HyperBrain]

I would check the populate call for a fulfillment just for the sake of completeness and verification that the newProperty result can be tested:

return expect(serverless.variables.populateProperty(property)).to.be.fulfilled
  .then(newProperty => ...

[lorengordon]

@HyperBrain I just tried that but get, TypeError: Cannot read property 'then' of undefined. The setup below does seem to work though. Does this do what you had in mind?

      return expect(serverless.variables.populateProperty(property).then(newProperty => {
        expect(newProperty).to.equal('my stage is prod');

        return BbPromise.resolve();
      })).to.be.fulfilled;

[HyperBrain]

@lorengordon Can you check if at the top of the file there is a chai.use(require('chai-as-promised'));?

This enables promise handling and testing, exactly as from my example above. The trick is, that you validate the promise, before you "then" it. "fulfilled" is only available when chai-as-promised is loaded as chai plugin.

This also enabled rejection testing with .rejectedWith(string or regex) which is in turn thenable again ;-)

[lorengordon]

This is the only line that contains chai:

const expect = require('chai').expect;

[HyperBrain]

Ok, then replace it with:

const chai = require('chai');

chai.use(require('chai-as-promised'));
const expect = chai.expect;

and we should have full chai promise support afterwards.
If there are sinon tests in the unit tests, you cann add chai.use(require('sinon-chai'));, to have the sinon extensions in place too. Then you can do things like expect(myStub).to.have.been.calledOnce.

[lorengordon]

Ok, this diff works using the approach you proposed:

diff --git a/lib/classes/Variables.test.js b/lib/classes/Variables.test.js
index 19943db7..10286531 100644
--- a/lib/classes/Variables.test.js
+++ b/lib/classes/Variables.test.js
@@ -3,7 +3,7 @@
 const path = require('path');
 const proxyquire = require('proxyquire');
 const YAML = require('js-yaml');
-const expect = require('chai').expect;
+const chai = require('chai');
 const Variables = require('../../lib/classes/Variables');
 const Utils = require('../../lib/classes/Utils');
 const Serverless = require('../../lib/Serverless');
@@ -14,6 +14,10 @@ const AwsProvider = require('../plugins/aws/provider/awsProvider');
 const BbPromise = require('bluebird');
 const os = require('os');

+chai.use(require('chai-as-promised'));
+
+const expect = chai.expect;
+
 describe('Variables', () => {
   describe('#constructor()', () => {
     const serverless = new Serverless();
@@ -182,11 +186,12 @@ describe('Variables', () => {

       serverless.variables.loadVariableSyntax();

-      return serverless.variables.populateProperty(property).then(newProperty => {
-        expect(newProperty).to.equal('my stage is prod');
-
-        return BbPromise.resolve();
-      });
+      return expect(serverless.variables.populateProperty(property)).to.be.fulfilled
+        .then(newProperty => {
+          expect(newProperty).to.equal('my stage is prod');
+        });
     });

     it('should call getValueFromSource if no overwrite syntax provided', () => {

[HyperBrain]

You can even dismiss the return BbPromise.resolve() if you just return the expect() because chai-as-promised turns the expectation result into promises:

 .then(newProperty => expect(newProperty).to.equal('my stage is prod'));

Sorry for the lots of single small comments from my side 😃

[lorengordon]

@HyperBrain I did remove that in the actual commit, and I modified the diff in the comment after the fact...

And no problem on the comments, I appreciate the pointers! Very new to javascript and much of this promise stuff escapes me!

[eahefnawy]

@lorengordon thanks for that! I see that you've modified some of our tests, but I think it's best if we add a separate test case that specifically tests this edge case. Imo it'd be better to keep our tests focused and high in quality.

Other than that, I've tried your reproduction steps, and they work as expected. 👍

[lorengordon]

@eahefnawy The tests I modified were invalid. They didn't actually test the primary condition they claimed to test. One of the tests already claimed to be checking for exactly what this patch fixes. I fixed the invalid tests. Now they actually do what they describe.

Please take another look at the modified tests and the discussion above.

Also, I'm afraid several other tests in this module may suffer from the same circular logic. 😢

[pmuens]

Thanks for providing this fix @lorengordon 👍

I just tested it and it works great! I also looked into the tests and they look legit 💯

Also, I'm afraid several other tests in this module may suffer from the same circular logic. 😢

Yes, that's also smth. I'm afraid of. Maybe it's a good idea to revisit the tests here soon.