Fix using dashes in AWS resource tags, add unit tests

[Jalle19]

This regular expression has been broken before, it's time it gets tested properly

Closes: #8764

I'm not sure whether naming.js is the best location to move the regular expression to, but it was the best I could find given that I'm not familiar with the codebase.

[codecov]

Codecov Report

Merging #8766 (4e25b2b) into master (d1c6568) will increase coverage by 0.17%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #8766      +/-   ##
==========================================
+ Coverage   87.60%   87.77%   +0.17%     
==========================================
  Files         260      265       +5     
  Lines        9664     9913     +249     
==========================================
+ Hits         8466     8701     +235     
- Misses       1198     1212      +14     

Impacted Files	Coverage Δ
lib/plugins/aws/provider.js	94.76% <ø> (+1.67%)	⬆️
...ib/plugins/aws/utils/getLambdaLayerArtifactPath.js	83.33% <0.00%> (-16.67%)	⬇️
lib/Serverless.js	92.24% <0.00%> (-4.93%)	⬇️
lib/utils/createFromTemplate.js	84.84% <0.00%> (-2.00%)	⬇️
lib/plugins/aws/lib/naming.js	97.67% <0.00%> (-0.52%)	⬇️
lib/plugins/package/lib/packageService.js	95.95% <0.00%> (-0.47%)	⬇️
lib/plugins/plugin/lib/utils.js	97.22% <0.00%> (-0.15%)	⬇️
lib/classes/PluginManager.js	97.21% <0.00%> (-0.09%)	⬇️
lib/plugins/print.js	100.00% <0.00%> (ø)
lib/utils/autocomplete.js	0.00% <0.00%> (ø)
... and 23 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1c6568...4e25b2b. Read the comment docs.

[Jalle19]

Added tests for the 128 character maximum length too

[Jalle19]

@pgrzesik escaping the dash did the trick indeed. The dot needed escaping too (I'm actually not 100% sure about that, but it might be safest to escape it anyway to avoid confusion).

[pgrzesik]

Thanks @Jalle19 for submitting this PR 🙇 I'd like to wait for @medikoo's response before moving forward as I'm not sure yet about the approach

[Jalle19]

I found #8051, but I can't dig up the tests that were inevitably scrapped from the pull request. Maybe reconsider not testing these validations at all? Clearly they are the source of multiple bugs, and complicating testing makes the barrier to entry for fixing them higher. I mean, I've already fixed the bug and written tests.

I agree that simple validations don't need testing because the logic is part of ajv, but regular expressions are part of this code and should be tested here.

[medikoo]

Maybe reconsider not testing these validations at all?

We don't want to fall into practice of writing tests for regular expressions confirming that they support all intended notations.
Value of such tests is controversial and they rise maintenance cost of already big project.

Approach we have is that we test properties of which format is validated through regular expression.

Here the real problem is not that regex was invalid, but that we didn't support given notation for a property, and that should simply addressed by (1) improving the regex which backs validation of that property, and (2) ensuring a regression test, that confirms, that given notation is now supported for given property.

So when testing is concerned I would simply add previously not supported notation e.g. in this test:

serverless/test/unit/lib/plugins/aws/package/compile/functions.test.js

Lines 379 to 414 in 420e937

	it('should create a function resource with function level tags', () => {
	const s3Folder = awsCompileFunctions.serverless.service.package.artifactDirectoryName;
	const s3FileName = awsCompileFunctions.serverless.service.package.artifact
	.split(path.sep)
	.pop();
	awsCompileFunctions.serverless.service.functions = {
	func: {
	handler: 'func.function.handler',
	name: 'new-service-dev-func',
	tags: {
	foo: 'bar',
	baz: 'qux',
	},
	},
	};
	const compiledFunction = {
	Type: 'AWS::Lambda::Function',
	DependsOn: ['FuncLogGroup'],
	Properties: {
	Code: {
	S3Key: `${s3Folder}/${s3FileName}`,
	S3Bucket: { Ref: 'ServerlessDeploymentBucket' },
	},
	FunctionName: 'new-service-dev-func',
	Handler: 'func.function.handler',
	MemorySize: 1024,
	Role: { 'Fn::GetAtt': ['IamRoleLambdaExecution', 'Arn'] },
	Runtime: 'nodejs12.x',
	Timeout: 6,
	Tags: [
	{ Key: 'foo', Value: 'bar' },
	{ Key: 'baz', Value: 'qux' },
	],
	},
	};

we may also add a comment there that it tests regression reported at {github link} (so at some cleanup by accident it's not removed)

[Jalle19]

@medikoo I changed the way the test is done, please review again

[medikoo]

Thank you @Jalle19 ! It's way better, I've just proposed a different approach to testing, which we practice in context of this project.

[Jalle19]

@medikoo I removed the tests and added just a few more defined tags to the existing test case. I'm not a fan of this solution since it can only test that valid values are allowed, not that invalid values are disallowed.

[medikoo]

Thank you @Jalle19 ! It looks great now, I've just proposed to reduce noise in regex and clear obsolete escape

[Jalle19]

This should be good to go now IMO, can't make the change any smaller

[medikoo]

Thank you @Jalle19, looks great!