Refactor `plugin-manager.test.js` #11861

sleepwithcoffee · 2023-03-25T03:44:47Z

Related issues: #8603 #7171

This is a follow up of PR #11853
Under this PR, I refactor plugin-manager.test.js to conform with several common code conventions

remove .bind(this) if arrow function is already sufficient
Use to.eventually.be.rejectedWith instead of to.be.rejectedWith, more verbose but screams out promise
Use to.eventually.be.fulfilled instead of to.not.be.rejected
Favor await over promises, unless it is a one-line return statement (best practice is always using await within function brackets)
Avoid being verbose in most cases

sleepwithcoffee · 2023-03-25T03:46:32Z

weird failed test. I tried to run in my local

npx mocha test/unit/lib/configuration/resolve-provider-name.test.js

  test/unit/lib/configuration/resolve-provider-name.test.js
    ✔ should read name from "provider"
    ✔ should read name from "provider.name"
    ✔ should reject missing "provider.name"
    ✔ should reject invalid "provider.name"
    ✔ should reject missing "provider"


  5 passing (13ms)

node --version (also tested with node v12, v14 and v16)

v16.19.1

sleepwithcoffee · 2023-03-25T05:30:30Z

reverted the troubled piece of code

codecov · 2023-03-25T05:39:25Z

Codecov Report

Patch and project coverage have no change.

Comparison is base (048ffad) 86.61% compared to head (0ad4b46) 86.61%.

❗ Current head 0ad4b46 differs from pull request most recent head 49b37e0. Consider uploading reports for the commit 49b37e0 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #11861   +/-   ##
=======================================
  Coverage   86.61%   86.61%           
=======================================
  Files         314      314           
  Lines       13181    13181           
=======================================
  Hits        11417    11417           
  Misses       1764     1764

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

medikoo

@sleepwithcoffee great thanks for the effort, still let's not adjust the old tests like that.

If we're about to do any work of them, they should be refactored to runServerless usage, See https://github.com/serverless/serverless/tree/main/test#unit-tests

test/unit/lib/classes/plugin-manager.test.js

sleepwithcoffee · 2023-03-27T12:41:13Z

hi @medikoo these are only meant to be syntactical changes, I do not intend to change the way they work (yet).

medikoo · 2023-03-27T13:31:30Z

hi @medikoo these are only meant to be syntactical changes, I do not intend to change the way they work (yet).

Thanks @sleepwithcoffee. Let's not introduce such syntactical changes. They do not seem to add value, while there's extra maintenance cost added

sleepwithcoffee · 2023-03-27T14:02:01Z

sure, maybe let me scope down this PR a bit. Some of them are still quite meaningful including highlighting async functions that we might have missed out earlier.
Another meaningful change is to remove sinon-chai by using built-in alternative in chai alone.

medikoo · 2023-03-27T14:05:04Z

sure, maybe let me scope down this PR a bit. Some of them are still quite meaningful including highlighting async functions that we might have missed out earlier.
Another meaningful change is to remove sinon-chai by using built-in alternative in chai alone.

@sleepwithcoffee you're doing great work here. What's important in case of old tests (those not configured with runServerlss), they're scheduled to be removed in favor of runServerless counterparts. So it makes not much sense to work on improvements to those, as it'll anyway be deleted.

Best improvement would be to simply replace them with runServerless based variants as we did in many other files already. Such contribution is very highly welcome.

…ss it is one-liner return remove `sinon-chai`from this file

sleepwithcoffee · 2023-03-27T15:23:02Z

reverted all minor syntax changes, focuses on 3 main points:

Favor await over promises, unless it is a one-line return statement (best practice is always using await within function brackets)
Use to.eventually.be.rejectedWith instead of to.be.rejectedWith, more verbose but screams out promise
Use to.eventually.be.fulfilled instead of to.not.be.rejected. Also only use this when it is one-liner return of promise, otherwise, use await
Remove the dependency of sinon-chai

medikoo

@sleepwithcoffee Thanks for this input, still again, let's refrain from refactoring old tests.

As I mentioned I'll be happy to welcome PR that coverts them to runServerless util.

Otherwise the only changes that are acceptable are those where e.g. we remove the dependency as blubird

medikoo · 2023-03-27T15:39:11Z

test/unit/lib/classes/plugin-manager.test.js

-      });
+
+      await pluginManager.asyncPluginInit();
+      expect(plugin1.asyncInit.calledOnce).to.be.true;


Let's not introce such stylistic changes (to.equal(true) into to.be.true), it brings on extra value, while increases maintenance cost

medikoo · 2023-03-27T15:39:33Z

test/unit/lib/classes/plugin-manager.test.js

      const servicePlugins = ['ServicePluginMock3', 'ServicePluginMock1'];

-      return expect(pluginManager.loadAllPlugins(servicePlugins)).to.be.rejectedWith(
+      await expect(pluginManager.loadAllPlugins(servicePlugins)).to.eventually.be.rejectedWith(


If to.be.rejectedWith works, let's keep it that way

medikoo · 2023-03-27T15:40:29Z

test/unit/lib/classes/plugin-manager.test.js

-    });
+    // Happens when `plugins` property exists but is empty
+    it('should not error if plugins = null', async () =>
+      expect(pluginManager.resolveServicePlugins(null)).to.eventually.be.fulfilled);


Let's not use be.fulfilled (as we agreed in other PR, let's just await promise in such case)

medikoo · 2023-03-27T15:41:10Z

test/unit/lib/classes/plugin-manager.test.js

@@ -852,23 +846,23 @@ describe('PluginManager', () => {
          },
        };
        expect(() => pluginManager.createCommandAlias('cmd1:alias2', 'mycmd')).to.throw(
-          /Alias "cmd1:alias2" is already defined/
+          'Alias "cmd1:alias2" is already defined'


Lert's not introduce such stylistic changes (as mentioned before, those tests are scheduled to be removed, we should increase maintenance cost of them)

sleepwithcoffee · 2023-03-30T13:16:35Z

I am closing this PR

sleepwithcoffee added 3 commits March 23, 2023 16:11

Merge remote-tracking branch 'upstream/main' into main

cda7059

Merge remote-tracking branch 'upstream/main' into main

76d0361

Merge tag 'v3.29.0' into main

d0c578c

sleepwithcoffee mentioned this pull request Mar 25, 2023

Refactor plugin-manager.test.js (obsolete) #11860

Closed

medikoo suggested changes Mar 27, 2023

View reviewed changes

test/unit/lib/classes/plugin-manager.test.js Outdated Show resolved Hide resolved

test/unit/lib/classes/plugin-manager.test.js Outdated Show resolved Hide resolved

medikoo assigned sleepwithcoffee Mar 27, 2023

medikoo added refactor tests labels Mar 27, 2023

refactor plugin-manager to favor await over returning promise, unle…

49b37e0

…ss it is one-liner return remove `sinon-chai`from this file

sleepwithcoffee force-pushed the 8603_refactor_pluginmanagertest branch from 3a4a53d to 49b37e0 Compare March 27, 2023 15:19

sleepwithcoffee requested a review from medikoo March 27, 2023 15:23

medikoo suggested changes Mar 27, 2023

View reviewed changes

sleepwithcoffee closed this Mar 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `plugin-manager.test.js` #11861

Refactor `plugin-manager.test.js` #11861

sleepwithcoffee commented Mar 25, 2023 •

edited

sleepwithcoffee commented Mar 25, 2023

sleepwithcoffee commented Mar 25, 2023

codecov bot commented Mar 25, 2023 •

edited

medikoo left a comment

sleepwithcoffee commented Mar 27, 2023

medikoo commented Mar 27, 2023

sleepwithcoffee commented Mar 27, 2023

medikoo commented Mar 27, 2023

sleepwithcoffee commented Mar 27, 2023 •

edited

medikoo left a comment

medikoo Mar 27, 2023

medikoo Mar 27, 2023

medikoo Mar 27, 2023

medikoo Mar 27, 2023

sleepwithcoffee commented Mar 30, 2023

Refactor plugin-manager.test.js #11861

Refactor plugin-manager.test.js #11861

Conversation

sleepwithcoffee commented Mar 25, 2023 • edited

sleepwithcoffee commented Mar 25, 2023

sleepwithcoffee commented Mar 25, 2023

codecov bot commented Mar 25, 2023 • edited

Codecov Report

medikoo left a comment

Choose a reason for hiding this comment

sleepwithcoffee commented Mar 27, 2023

medikoo commented Mar 27, 2023

sleepwithcoffee commented Mar 27, 2023

medikoo commented Mar 27, 2023

sleepwithcoffee commented Mar 27, 2023 • edited

medikoo left a comment

Choose a reason for hiding this comment

medikoo Mar 27, 2023

Choose a reason for hiding this comment

medikoo Mar 27, 2023

Choose a reason for hiding this comment

medikoo Mar 27, 2023

Choose a reason for hiding this comment

medikoo Mar 27, 2023

Choose a reason for hiding this comment

sleepwithcoffee commented Mar 30, 2023

Refactor `plugin-manager.test.js` #11861

Refactor `plugin-manager.test.js` #11861

sleepwithcoffee commented Mar 25, 2023 •

edited

codecov bot commented Mar 25, 2023 •

edited

sleepwithcoffee commented Mar 27, 2023 •

edited