refactor: replace log,exit in utils of init,addons command

[tinfoil-knight]

- Summary

#3028 was too large for review and is being broken down.

This PR is a follow-up on #3247 and uses the separate logging and process control utilities created in it to remove the inherited ones.

This PR focuses mostly on utilities related to the init,addons commands. Changes are a bit sparse and not contained in a specific command folder like the previous PRs.

Also the detectAwsSdkError function in src/lib/functions/utils.js was unused and has been removed in a separate commit. If this change was not supposed to be made (or be done in a separate PR), we could just remove that commit.

This should be the last PR needed to close PR #3028 and issue #2728.

- Test plan

npx ava --verbose --serial

[ehmicky]

Should the argument warn be removed altogether?

It seems like the reason this is here is because dot-env.test.js is mocking warn(). This is probably to remove warning messages printed during tests. However, mocking seems the wrong way to solve this, as it would not scale well: many test files would need to mock warn().

Based on this, what are your thoughts on the following?

• Removing the warnLog/warn argument from this function.
• Import the warn utility directly instead
• In dot-env.test.js, do not pass any warn stub

Additionally, splitting the function into two as suggested above could help removing the warning message from the unit tests.

(Note: this specific change might deserve its own PR)

[tinfoil-knight]

I'd definitely like to remove the warn argument.

Internally in @oclif/errors, warn relies on console.error for logging but I don't think we should rely on that to change the global object.

Also, AFAIK in some places the logs are tested which means that we can't use the test environment flag to disable them.
(But, we could use an alternate environment variable which can be set before tests that require logging to be disabled and removed during cleanup.)

In dot-env.test.js, do not pass any warn stub

I went through the tests and they only seem to be checking the results. Why was warn being suppressed in that case?

Additionally, splitting the function into two as suggested above could help removing the warning message from the unit tests.

Yeah. We could do this.

[ehmicky]

Why was warn being suppressed in that case?

My guess is that this was to remove the verbosity of the console output when running tests. However, splitting the logging logic to a parent function would allow the tests to import the core logic without printing any log messages.

[ehmicky]

There might be some opportunity to improve how this function is being tested.

At the moment, we mock exit, although we don't really need to. Instead, we could split getAgent() into two functions:

• tryGetAgent({ httpProxy, certificateFile }), returning an object either { agent } (success) or { agent, warning: 'warning message' } or { error: 'error message' }
• getAgent({ httpProxy, certificateFile }), the top-level function, would call tryGetAgent(). If it returns:
  • an error, it would log() it with NETLIFYDEVERR and call exit(1)
  • a warning, it would log() it with NETLIFYDEVWARN but not exit()
  • an agent, it would return it

This approach would have the benefits of:

• Decoupling logging/exiting from the main actual logic
• Removing the need to mock exit() in the unit tests, since those could test tryGetAgent() instead

What do you think?

(Note: this specific change might deserve its own PR)

[tinfoil-knight]

I think it's better to check function responses rather than the behaviour of process control and logging utilities.

One thing is that it'd create an export which won't be used anywhere except for the tests. Is that okay?

[ehmicky]

One thing is that it'd create an export which won't be used anywhere except for the tests. Is that okay?

I think this is a side effect of unit testing (as opposed to integration testing). Since we are testing at a small scale, we sometimes need to hook into specific functions, i.e. create exports for them just for testing purpose.

Even if we were to enforce not creating exports just for unit tests, we would still do something quite similar: we would let test files import production files that are not imported directly by end users. To illustrate how this is in essence the same thing: in the case above, we could theoretically split those two functions into separate files, so that the unit test does not create a new export (although that'd be artificial).

I think it's the nature of unit testing to break encapsulation for testing purpose. It's also its major drawback (making it more brittle and implementation-specific) but also advantage (making it simpler), compared to integration testing.

[tinfoil-knight]

Gotcha!

[tinfoil-knight]

@ehmicky I'm guessing we're making the suggested changes in another PR as mentioned in the comments or are any changes needed in this one itself?

[ehmicky]

I would think of splitting the dot-env.js and http-agent.js into 2 separate PRs. But the other changes in this PR are still good to go (after removing those two files' changes)?