New Placeholder Unit Tests for future creation #2767
Conversation
|
Nice one @pkbullock . Have you written some kind of code generator to generate these stubs? If so, would it be possible to run it again removing the empty lines so all unit tests look perfect from the start? |
|
@KoenZomers - sorry bit slow to respond, work is crazy busy this week. Yes, I wrote a quick generator to stub out all the tests based on the cmdlets and grabbed the input parameters, put into folders etc. Happy to do some tidying up, I need to fix a few things, the Web cmdlets are not part of the same namespace format and my tool excluded those locations. If I see a common pattern with the tests I will look to generate those out if possible to reduce the implementation time. I'm prob going to re-write some of the tests as well, there is a block that generates SharePoint site collections for each test adding around 4.5 min per test. Are there any DevOps style auto builds or processes that might be impacted by this change? |
|
@pkbullock, nope, no DevOps automation exists currently. As I understood the idea from @erwinvanhunen, no actual data would be sent to any SharePoint tenant with the way these unit tests would be set up. It would merely check the request that would be sent to SharePoint in a stub and return the expected value to the code. |
|
@KoenZomers the work I am doing isn't that solution at this time. I am aware of a similar technique (although not CSOM) that is used within the new https://github.com/pnp/pnpcore project to mock the calls. Even so, there is some dependency on a tenant to initially generate the mock calls and periodically regenerate them. What i am referring to is that there is a set of tests current that operate in a way that is very expensive and unnecessary, e.g 10 tests each creating a new site collection to test copy/move cmdlets each test has a cost of 4.5 min - to optimise the whole test rig can have the requirement (documented and referenced in appsettings) to have 2 or more site collections, thus avoiding the creation process. |
|
@pkbullock I do think, and that's what I also understood from @erwinvanhunen, is that we do want to use the same mocking as you describe in PnP Core. A Unit Test that would test against a live tenant and live data which would in my opinion be wasted efforts. @erwinvanhunen please chime in here with your thoughts on the subject. |
|
That is indeed the case, @KoenZomers. However, in order to pull that off I would like to wait for the PnP.Net SDK team to finalize their efforts there, as there are still a few things to figure out. The moment they feel it's working as they want we can start to look into doing this for PnP PowerShell. Having said that, a restructuring of the unit tests for PnP PowerShell, as @pkbullock did here makes absolutely sense. |
|
@erwinvanhunen So are you saying we should invest time now to build unit tests against live data and once the Core team is done switch it all to the stubs? Feels like wasted time to me. Shouldn't we just wait for them to be done and THEN start building these unit tests? |
|
So as far as I understand (based on how pnpcore works) you still need the live connections anyway, firstly to generate the stubs because it listens and records in files the API calls sent and responses from SP CSOM/Rest/Graph. Any updates you make to the code or SharePoint API changes would require a refresh from the live connection. Tests would eventually have a one line switch to either use live or stubbed connection. In the short term, I have happy to write out the tests just so we have something in place - get the test coverage up. I am anticipating this wouldn't be substantial rewrite based on what I have seen so far if it is. I'll make sure any common validation code is centralised to reduce any future pain. I can to move to that model once it has been proven or accepted - which it may not be or its too complex based on the number of API calls made. If there are any obvious quick wins in performance as discussed previously Ill implement those include any documentation and notes in future PRs. Is that ok? |
|
@KoenZomers - i have added the formatting changes and some inline param help to make it easier to understand the parameters based on the same help of the cmdlet. Please let me know what you think? |
|
Looks good to me! I had a quick call with @erwinvanhunen on this topic yesterday. I think it's super useful to get this in place. From here let's wait until PnP Core is done sorting out how to unit test using CSOM and then we can start implementing the actual test logic in these classes. One thing we would like to have clarified from you still is this remark:
What exactly does that mean? Can we accept this PR as it is? It's way too big to validate manually. Or do you mean we should not accept some of the changes in this PR? Please clarify. |
|
ok cool, I'll hold off writing anymore. In regards to "Note: I made some errors in my branches with commits efd14c1 and 30cb6b4 - I don't want to include them at this time. Sorry my git skills on the light side." - have reverted these commits in 2999856 and 4a63f50 undoing a previous change (show above, just before the auto-assign bot) that I made earlier in the month. This will mean the PR is good to go as is. |
Type
This PR includes test placeholders and some organisation of tests.
The tests include some help to get started with calling the cmdlet and parameters based on the list of existing cmdlets. This should save some coding time and hopefully encourage others to assist - i will still continue to write these out fully.
Note: I made some errors in my branches with commits efd14c1 and 30cb6b4 - I don't want to include them at this time. Sorry my git skills on the light side.