Implement Fn::Split, remove cloudformation-js-yaml-schema #148
Conversation
akdor1154
changed the title
There was a problem hiding this comment.
This is great!
Beautiful tests, is there a way of splitting these up?
I think these type of functional unit tests are the way forward. It's still nice to show templates working however.
Splitting these into two different test runs now would be neat (and set a standard for future work), then we can have some future work for true unit code coverage and reporting.
src/test/validatorTest.ts
Outdated
| @@ -175,99 +175,99 @@ describe('validator', () => { | |||
| let result = validator.validateJsonObject(input); | |||
| expect(result).to.have.deep.property('templateValid', false); | |||
| expect(result['errors']['crit']).to.have.lengthOf(1); | |||
There was a problem hiding this comment.
Can you revert the whitespace changes?
There was a problem hiding this comment.
sure, just to keep it out of the merge commit for this pr?
There was a problem hiding this comment.
Yep, I don't mind individual reformatting PRs but they shouldn't change functionality or its a nightmare when looking at git history
| it('should resolve an intrinsic function', () => { | ||
| const input = { | ||
| 'Fn::Split': ['-', { | ||
| 'Fn::Select': [1, ['0-0', '1-1', '2-2']] |
|
Re tests - yeah I think it's a good pattern to have both. I want to do a bit of refactoring of the other intrinsic functions as well though, as it is it's easy to test success cases but a bit harder to test invalid cases. You'll note that I was unable to test specific error paths got taken for the 'invalid' tests. I'm looking somewhat towards the pattern function runFn() {
try {
return doFn();
} catch (e) {
addError('crit', e.message);
return 'fnInvalid';
}
}
function doFn() {
if (bad input) { throw new Error('error message') }
return (implement actual function in here);
}That way |
|
sorry, to be explicit - can I leave the tests the way they are now, and refactor them along with the intrinsics in a later pr? |
|
@akdor1154 Yeah I see where you are coming from, I'm happy with that - seems a good way forward. |
This PR implements and tests Fn::Split.
I couldn't do this to begin with as cloudformation-js-yaml-schema does not have a working version available. The current dependency 0.3.1 does not recognize !Split, and the latest version 0.3.4 does not recognize !ImportValue when its argument is a string.
Looking through the source of cloudformation-js-yaml-schema, I believe it will cause yaml parse errors on other inputs as well which might be the source of other bugs. It assumes that arguments to !Tags are a certain type (string, array, obj), and throws a parse error if the type doesn't match. We should be (and generally are) doing that check ourself so we can give better feedback to the user - we should be saying "You can't pass a string to Fn::Join", not "Yaml parse error".
However, we can do everything it does easily using the spec files we already have at our disposal. This also significantly cleans up parser.ts as a result, yay!