pnp
Centralize core command logic in mocha hooks. Closes #4901
Closes #4901
A way to centralize some code.
I had a test coverage issue with the
logRawfunction of the Logger interface. I checked and we're only using it once. I made thelogRawfunction optional to work around the issue.
Nice! Awesome to see how much code we can avoid repeating. Let's see how it would work with tests ran in parallel so that if we choose to go down that path, we're not blocked.
@martinlingstuyl looks really promising 👍 I think it will be a significant improvement in the codebase 💪. checked locally ✅
@pnp/cli-for-microsoft-365-maintainers what do you think? I am all in 🤩
One last comments (see above). Other than that, I think we're in a good shape. Even though we're centralizing some code, in case we need to handle an exception, we can choose to opt-out of the centralized code, so we should be able to support all scenarios that we need.
Hmmm... I now have an issue with a test coverage on logger.logRaw. Need to fix that first...
Marking it as draft until you do @martinlingstuyl to ensure we won't merge it by accident
Ok, @waldekmastykarz, I had a test coverage issue with the logRaw function. I made it optional on the Logger interface to work around the issue. We're not really using it by the way.. just one time in a mock command.
Ok, @waldekmastykarz, I had a test coverage issue with the logRaw function. I made it optional on the Logger interface to work around the issue. We're not really using it by the way.. just one time in a mock command.
If we're not using it, let's remove it to avoid keeping unnecessary code. Not sure if we needed it in the past for something, but if we don't need it at the moment, let's remove it. If a need for it arises in the future, we can always add it back.
Have you ever looked at parallel execution @martinlingstuyl? I see a potential issue if we're tracking things like log or spies centrally in the module. If the module is reused and one test resets the spy, why the other needs it, it would lead to conflicts and prevent us from using parallel execution. I wonder if we should look into other approaches like having a function that returns a set of mocks that are scoped in the test suites and are independent of other instances of the central module used in other suites.
Hi @waldekmastykarz, I have looked into it, but could not try it out easily because it's currently not functioning in our project. So I parked it...
Thinking about it, you're right: the module is instantiated at load time, right? Meaning every test gets the same variable instances. I had not realized that, but that sure is an issue.
Let me look into it.
@martinlingstuyl you can easily test it on CLI by adding --parallel to the mocha command.
@martinlingstuyl you can easily test it on CLI by adding
--parallelto the mocha command.
Of course, I have done that. But it doesn't currently work, breaks some tests for some reasons. Sometimes it breaks one, then it breaks some other one... we have some work to do before we can make this work for us.
@martinlingstuyl you can easily test it on CLI by adding
--parallelto the mocha command.Of course, I have done that. But it doesn't currently work, breaks some tests for some reasons. Sometimes it breaks one, then it breaks some other one... we have some work to do before we can make this work for us.
Could be very much a symptom of dependencies/assumptions between test suites. Would be good to understand what's wrong, even if we won't fix it immediately because it's not related to this issue.
@waldekmastykarz, updates, updates...
I wonder if we should look into other approaches like having a function that returns a set of mocks that are scoped in the test suites and are independent of other instances of the central module used in other suites.
OK, I've changed the setup. I now run a function to get an instance of everything that I need, centralized as for code, but scoped in the test suite.
I'm not really enthousiastic about it, but I haven't found a better way till now.
I see a potential issue if we're tracking things like log or spies centrally in the module. If the module is reused and one test resets the spy, why the other needs it, it would lead to conflicts and prevent us from using parallel execution
However, I'm not sure this will solve everything related to parallel execution. For example: we set auth.service.connected to true in the before hook. Is auth not also a single instance here? If so, we'd get the same issue with other modules like these.
We could change auth to a mock instead of a stub. That way we could make sure we'd have a new instance every time...
I was wondering if you could maybe give a look see to this one @waldekmastykarz?
Should we put this on hold for now @waldekmastykarz, until there's more clarity on jest?
Should we put this on hold for now @waldekmastykarz, until there's more clarity on jest?
Let's. If we decide we want to migrate to Jest then we'll likely need to refactor this too.