Skip to content

Conversation

@sleepwithcoffee
Copy link
Contributor

Closes: #2582

  • I did refactoring from a simple assignment to copy this.options = Object.assign({}, options);
  • Also performed some minor refactoring along the way

Consideration for next step: create a base class for all plugins or groups of plugins e.g. AwsBasePlugin -> then in the base class constructor we initiate some basic properties rather than copy the same init code in all plugins.
This will avoid code duplication but the trade-off will be nested inheritance.

@sleepwithcoffee
Copy link
Contributor Author

seems that this runs deeper than I thought. Even in our code base, we tend to modify serverless and options after the fact.
We can refactor all test cases to move toward decoupling, however, I just want to make sure this is the path that we want to go.

On top of that, modifying this can possibly make other plugins break if they went for the 'hacky' way.

@medikoo
Copy link
Contributor

medikoo commented Mar 13, 2023

@sleepwithcoffee great thanks for giving that a spin!

seems that this runs deeper than I thought. Even in our code base, we tend to modify serverless and options after the fact.

When you spot such cases, do you think they intentionally update the global object, or it's more not envisioned side effect?

We can refactor all test cases to move toward decoupling, however, I just want to make sure this is the path that we want to go.

The ideal approach is to in first place, refactor all approached tests to rely on runServerless util (and ideally if each test file is covered with different PR). This can turn as bigger task, so maybe first let's list which of the failing tests happen in files that demand such refactoring. Some test files already have an existing guide on how to refactor them: https://github.com/serverless/serverless/issues?q=is%3Aopen+is%3Aissue+label%3Atests+label%3A%22good+first+issue%22)

@medikoo medikoo added the bug/design Functionality design flaw label Mar 13, 2023
@sleepwithcoffee
Copy link
Contributor Author

I would say we have to refactor to decouple the test cases first. Right now, it is critical to run tests in order due to the way objects are initialized which is not good at all!

My take is every test should be dependent from each other. Tt makes our tests less error-prone and compatible with parallelization.

@medikoo
Copy link
Contributor

medikoo commented Mar 13, 2023

@sleepwithcoffee yes, that's a big problem with old tests, which focused to much on confirming implementation details which should not be the case. Refactoring test to rely on runServerless (so treating implementation as black box) fixes that. At the same time it's fine to remove completely test, that focus purely on internal implementation characteristics and not the outcome visible for user

@sleepwithcoffee
Copy link
Contributor Author

sleepwithcoffee commented Mar 14, 2023

Created PR #11812 to address the issue with tests, can merge that one first before moving on this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug/design Functionality design flaw

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ensure options as passed to plugins is not modified

2 participants