test: Refactor tests to use env instead of opt vars
#672
Conversation
|
Hey @martinezpl, thanks for PR submission - it looks like the CI builds are failing - could you please look into that? |
|
Whoops, sorry, fixed. |
There was a problem hiding this comment.
thanks @martinezpl - I have one suggestion that could make the solution cleaner (imo), let me know what do you think
test.js
Outdated
| let envVars = { SLS_DEBUG: 't' }; | ||
| if (cmd == 'sls') { | ||
| // convert deprecated CLI arguments to env vars | ||
| const argsCopy = [...args]; |
There was a problem hiding this comment.
While this is a working solution, I think it would be better to explicitly pass these options as env to sls created with mkCommand utility. What do you think? For example, invocation could look like this:
sls(['package'], {env: {DOCKERIZE_PIP: true, SLIM: true}});
instead of
sls(['--dockerizePip=true', '--slim=true', 'package']);
There was a problem hiding this comment.
I totally agree, but I'd need to spend time on manually rewriting each function... ;/
Maybe I'll come up with a script to parse these lines for me if you insist on this - if you don't, then I think it's gonna be good to merge after I push the lint correction.
There was a problem hiding this comment.
Hey @martinezpl - I think we will need to address it anyway - if you won't have time to address it, I can give it a try, I'd rather not merge in code that needs to be changed soon. Let me know 💯
There was a problem hiding this comment.
Done, turned out to be a pretty quick task with a bit of regex.
-edit
I've run into some problems after implementing the convention you suggested - the sls command threw NOENT errors, that's why the code in my commit differs.
However now I realised I just cut out the way to pass options different than 'env'. Seems like it hasn't been used so far, but it's probably better to have it. Let me correct.
-edit2
Done!
pgrzesik
changed the title
env instead of opt vars
Hey,
I wanted to work on a different feature, but to my surprise most of the tests were failing on an unmodified fork. That's because I was using latest version of Serverless and the tests are full of CLI arguments which are now deprecated. Here's a small trick that makes the tests valid again without having to modify all the functions.
I also think getting rid of the python2.7 tests is a good idea.