Centralize some code used in tests

[martinlingstuyl]

Some things on our current mocha test setup:

1. There are quite a few lines of code that are duplicated in almost every spec.ts file.
2. Some things sometimes seem to bleed through into other test suites (like config keys or variables)

We would do well to refactor that a bit.

What can we do

• centralize some stubs in before hooks.
• centralize stub / spy restoration in after hooks.
• centralize the logger setup in (often) the beforeEach hook.
• Use default values for config keys and variables.

I propose that we do all this in the before and after hooks.

If in specific test suites these stubs or spies are overridden, that's OK, these test suites are then responsible for setting up the correct stubs for the entire suite.

You can check out the PR for the current implementation.

Centralized Stubs/Spies

• auth.restoreAuth
• telemetry.trackEvent
• pid.getProcessName
• session.getId
• cli.getSettingWithDefaultValue (returns the default value for every config key)
• logger.log
• logger.logToStderr

Settings

• auth.service.connected = true;
• auth.service.spoUrl = undefined;

[martinlingstuyl]

I did some ground work in the POC PR connected to this issue.

@waldekmastykarz, maybe we could also evaluate what kind of things we could add here additionally, to reach a 'default' setup. What about adding a good configuration stub with just the right default values? Like prompt = false, etc.

[waldekmastykarz]

I did some ground work in the POC PR connected to this issue.

@waldekmastykarz, maybe we could also evaluate what kind of things we could add here additionally, to reach a 'default' setup. What about adding a good configuration stub with just the right default values? Like prompt = false, etc.

Great idea! The more we can simplify the base setup, the easier it will be for our contributors to write tests. Let's look for ways to put them on the path to success 💪

[martinlingstuyl]

@waldekmastykarz: Also, I left out stubs that occur in the codebase less often (like the form request digest stub.)

We could add it of course, but that would lead to a lot of unnecessary stub initialization.

[waldekmastykarz]

@waldekmastykarz: Also, I left out stubs that occur in the codebase less often (like the form request digest stub.)

We could add it of course, but that would lead to a lot of unnecessary stub initialization.

We could of course have another function, specific with CSOM-related stubs that we invoke in all CSOM-related tests. That way we can avoid repetition without unnecessarily initiating stubs that we don't need 😎

[martinlingstuyl]

Or we could use parameters to include extra stubs. Your preference is extra functions?

[martinlingstuyl]

We could of course have another function, specific with CSOM-related stubs that we invoke in all CSOM-related tests. That way we can avoid repetition without unnecessarily initiating stubs that we don't need 😎

I've adjusted the setup slightly, to be able to incorporate commands with prompt stubs and commands with requestDigest stubs. Which are both fairly common. What do you think of it like this?

Instead of centralizedBeforeHook(true), which is rather unclear, we could of course also employ options objects: centralizedBeforeHook({ includeRequestDigest: true })

[waldekmastykarz]

Or we could use parameters to include extra stubs. Your preference is extra functions?

Let's ensure that whatever approach we choose, it won't get too complicated to use and won't require us to write additional docs and folks to read them first.

[martinlingstuyl]

Agreed. Idea 💡 Maybe we should just use extra functions. And some renames may be in order as well:

before(() => {
    includeDefaultBeforeHookSetup();
    includeFormDigestStub();
});

beforeEach(() => {
    includeDefaultBeforeEachHookSetup();
    includePromptStub();
});

// ...etc

[martinlingstuyl]

How about this new setup peeps? @pnp/cli-for-microsoft-365-maintainers?

I'll update the PR after the holidays. The full work however should still be done

[Adam-it]

includeDefaultBeforeEachHookSetup();
    includePromptStub();

@martinlingstuyl I think that's a good idea but let's not make it too granular. Otherwise we will get back to the point when most of the test have the same include... stub methods.

[martinlingstuyl]

On hold till we've thought some more about implementing jest

[milanholemans]

@pnp/cli-for-microsoft-365-maintainers Should we unblock this issue since we are staying with Mocha?