search

serverless / serverless

⚡ Serverless Framework – Effortlessly build apps that auto-scale, incur zero costs when idle, and require minimal maintenance using AWS Lambda and other managed cloud services.
https://serverless.com
MIT License
46.5k stars 5.73k forks source link

Tests: Refactor lib/plugins/aws/package/compile/functions/index.test.js #8523

Open medikoo opened 4 years ago

medikoo commented 4 years ago

Most of the tests that currently cover core functionalities depend heavily on (and sometimes test) internal implementation characteristics. While they should be testing whether given implementation produces desired outcome (treating its implementation more as a black box).

Current state of things is problematic for eventual internal improvements and refactors which occasionally we want to introduce, as in most of such cases proposed improvements need to be accompanied with counterproductive numerous updates to tests which are covering otherwise not altered functionalities .

At some point we've introduced a new (black box based) way of testing the internals. It's through runServerless utlity, which allows us create natural (as in real world) serverless instance, and inspect the produced outcome for chosen command. More details here: https://github.com/serverless/serverless/tree/master/test#unit-tests

This is issue is about refactoring lib/plugins/aws/package/compile/functions/index.test.js to runServerless based variant.

It's needed, so we cleanly move forward with https://github.com/serverless/serverless/issues/8396

To make refactor relatively easy:

For every refactored test, the old test should be removed

As this test file is large, let's divide work into following parts (best if each part is addressed with individual PR):

pgrzesik commented 3 years ago

Hello @medikoo :wave: I had some free time so I picked up the first section of refactoring: Provider properties - while it's mostly self-explanatory, I've stumbled upon one thing that I'd like to clarify before moving forward with any decisions (and to ensure that my thinking is correct).

The test case that I'd like to ask about is called 'TODO: should support package.deploymentBucket and it references existing test: https://github.com/serverless/serverless/blob/d8527d8b57e7e5f0b94ba704d9f53adb34298d99/lib/plugins/aws/package/compile/functions/index.test.js#L1939-L1982 which sets the custom deploymentBucket on serverless.service.package, however, according to the current schema definition that property is not supported. In the production code in compile/functions/index.js, it still supports the logic for overriding bucket via package.deploymentBucket: https://github.com/serverless/serverless/blob/d8527d8b57e7e5f0b94ba704d9f53adb34298d99/lib/plugins/aws/package/compile/functions/index.js#L149-L151 which is first set in lib/classes/Service.js: https://github.com/serverless/serverless/blob/d8527d8b57e7e5f0b94ba704d9f53adb34298d99/lib/classes/Service.js#L168-L170

Sorry for a bit lengthy explanation, but I wanted to share more context about the following question - should the package.deploymentBucket be supported which in turn means adjusting schema so it can accept it, or should the support for that notation be dropped (or it was silently dropped previously but there is still some leftover code)?

Thanks in advance :bow:

medikoo commented 3 years ago

@pgrzesik Great thanks for that! and you've raised a very valid question

which sets the custom deploymentBucket on serverless.service.package, however, according to the current schema definition that property is not supported

Yes, I overlooked that when specifying the test. Thing is that Internally provider.deploymentBucket is assigned to service.package.deploymentBucket, and why current test operate on service.package

So technically test title should be should support `provider.deploymentBucket`, and it's the property that should be set in runServerless run. Sorry for confusion

pgrzesik commented 3 years ago

Thanks a lot for explanation @medikoo :bow: What about supporting the package as the top-level property in serverlessFile? If I'm not mistaken, the code can handle that at the moment (see the linked snippets), but the schema definition prevents doing that - is that by design?

medikoo commented 3 years ago

Thanks a lot for explanation @medikoo 🙇 What about supporting the package as the top-level property in serverlessFile?

hmm.. package is supported and recognized as top level property -> https://github.com/serverless/serverless/blob/b12d565ea0ad588445fb120e049db157afc7bf37/lib/configSchema.js#L72

pgrzesik commented 3 years ago

That's correct, sorry for not being clear with my previous message, I took a mental shortcut which changed the meaning of my message significantly. What I wanted to highlight that it's technically possible (unless it gets overriden along the way and I missed that) to configure deploymentBucket on package so as package.deploymentBucket in serverlessFile, however, current schema does not list that field as available, but according to code in https://github.com/serverless/serverless/blob/d8527d8b57e7e5f0b94ba704d9f53adb34298d99/lib/classes/Service.js#L168-L170 it should be possible. Is that intended behavior or maybe I'm missing something and even if it would be allowed by schema, it would be overriden somewhere during processing?

medikoo commented 3 years ago

What I wanted to highlight that it's technically possible (unless it gets overriden along the way and I missed that) to configure deploymentBucket on package so as package.deploymentBucket in serverlessFile, however, current schema does not list that field as available, but according to code in

Ah yes, I think it's possible to find few more such "shortcuts". To me they're side effects of messy internals, and I don't think we should recognize such notation as valid (there should only one source of truth for every setting)

scadu commented 3 years ago

As discussed in https://github.com/serverless/serverless/pull/8891#discussion_r591185530, I'd cover refactor of tests required to merge #8891 with reliable tests.

js-cha commented 3 years ago

hello @medikoo I would like to cover some of the remaining refactoring tasks if that's okay?

pgrzesik commented 3 years ago

It would be very welcome @js-cha, thank you :bow:

issea1015 commented 3 years ago

I believe you've just missed it, so giving warm notice, it'd be valuable to mark Provider role variants part as done in the issue description so that we can track the progress. @pgrzesik Thanks ~

pgrzesik commented 3 years ago

Thanks for being vigilant and letting me know about it @issea1015 :bow:

sdas13 commented 2 years ago

Hi @pgrzesik can I pick up a few refactoring tasks?

pgrzesik commented 2 years ago

Sure @sdas13 - but I would hold off for about a week or two as we're freezing current master branch and releasing v3 in the coming days. After that release it will be safe to refactor, otherwise you might end up with a lot of extra changes.

sdas13 commented 2 years ago

Thanks @pgrzesik I will keep an eye on the v3 release

eli6 commented 2 years ago

Hi @pgrzesik / @medikoo ! I'd like to refactor the " Version hash resolution" part. Would that be ok?

pgrzesik commented 2 years ago

Sure - we'd be happy to accept PR for that @eli6 - thank you 🙇

eli6 commented 2 years ago

@pgrzesik I am also considering helping out with the section "Download package artifact from S3 bucket:". Is that ok as well?

In that case I have a question, what s3 urls should I put in the runServerless-configExt for the runServerless-call just above those tests (where the placeholders 'some s3 url' etc. are?)?

 configExt: {
          package: { artifact: 'some s3 url' },
          functions: { basic: { package: { individually: true, artifact: 'other s3 url' } } },
        }

I tried putting the same url as in the test that I should replace there, and run the empty tests, and I also added a awsRequestStubMap with (among others) a stubbed STS:getCallerIdentity as in the linked example, but I get a " Missing credentials in config" error when running the tests.

pgrzesik commented 2 years ago

Hey @eli6 - you can use any url you want but ensure stubbing the AWS calls as suggested in the comments for the test. If you're running into missing credentials issue it might be the case that some calls are not stubbed - feel free to share a draft PR and I can take a deeper look because it's hard to help out without seeing the code that is causing trouble