pnp / pnpjs

Fluent JavaScript API for SharePoint and Microsoft Graph REST APIs
https://pnp.github.io/pnpjs/
Other
763 stars 304 forks source link

Add SPFx Webpart Harness for Testing #2690

Closed HiltonGiesenow closed 1 year ago

HiltonGiesenow commented 1 year ago

I've been working on my first PR for this repo and struggling to get a test scenario up and running as there are a lot of steps for what is/was a very simple bug fix. I wondered if it would be possible, considering this repo is largely for SharePoint anyway, to include some kind of SPFx web part testing harness, kind of similar to this: https://github.com/pnp/sp-dev-fx-property-controls/tree/master/src/webparts/propertyControlsTest . Obviously it wouldn't be identical because this repo isn't a series of webparts, but in essence just the core idea I mean.

Category

Version

Please specify what version of the library you are using: [3.15]

Please specify what version(s) of SharePoint you are targeting: [SPO]

HiltonGiesenow commented 1 year ago

I'd be happy to help with this, fyi

patrick-rodgers commented 1 year ago

Not entirely sure if this is what you mean, but you can run npm run spfx and it spins up this test spfx webpart which you can edit. All the refs are set such that the live builds of the library code are loaded in the webpart. Sometimes you have to edit the webpart itself to trigger the refresh, but otherwise the code is all watched.

We've not documented that command as it doesn't work 100% perfect for auto-reloads yet. If you're interested in improving what we've done your help is definitely appreciated!

HiltonGiesenow commented 1 year ago

Ok thanks Patrick, I'll check it out.

HiltonGiesenow commented 1 year ago

Hi @patrick-rodgers . I'm trying to get up and running with this but not getting too far. I'm adding the web part onto the workbench, but immediately get an error: Cannot access '_Web' before initialization. Any ideas?

bcameron1231 commented 1 year ago

We have a circular import issue currently that is causing this error I believe... (though it could also be a missing import).

// issue being fixed. https://github.com/pnp/pnpjs/pull/2682

I think it can be fixed by adjusting your imports. Can you share what your imports look like in your Web Part file?

HiltonGiesenow commented 1 year ago

Do you mean imports in HelloWorldWebPart.ts? Here's what I have:

`import { Version } from '@microsoft/sp-core-library'; import { BaseClientSideWebPart } from '@microsoft/sp-webpart-base';

import styles from './HelloWorldWebPart.module.scss';

import { getGUID } from "@pnp/core"; import "@pnp/sp/webs"; import { SPFI, SPFx, spfi } from '@pnp/sp'; `

It's 100% as in the repo right now - I haven't changed anything.

bcameron1231 commented 1 year ago

Hmm. Do you have other changes on the SPFx side? Or is the entire web part file as is from the repo?

HiltonGiesenow commented 1 year ago

Nope, no changes at all - just wanted to check it out to see if I could get it up and running.

bcameron1231 commented 1 year ago

Weird, okay. I'm having trouble repro'ing. (Though I have seen this before). We have seen issues with the SPFx solution (as Patrick eluded to earlier). Do you have changes to the core library in the same solution?

HiltonGiesenow commented 1 year ago

The only change is the fix I submitted the other day - https://github.com/pnp/pnpjs/pull/2689

bcameron1231 commented 1 year ago

Okay. I was able to repro.

Just humor me for a second.... can you delete the "@pnp/sp/webs" import? And re-add it under the "@pnp/sp" import (though order really doesn't matter). We believe this is all related to some circular reference issues (as linked above).

HiltonGiesenow commented 1 year ago

Nice! That seemed to fix it! I also wasn't aware there was an order required for the imports (I've always naturally import SPFI etc. first). I've submitted a PR for this now

martinlingstuyl commented 1 year ago

Hi @patrick-rodgers . I'm trying to get up and running with this but not getting too far. I'm adding the web part onto the workbench, but immediately get an error: Cannot access '_Web' before initialization. Any ideas?

I had the same and your idea fixed it @bcameron1231! 👏

bcameron1231 commented 1 year ago

For clarity, the order shouldn't matter . But I'm glad you got it working! :)

martinlingstuyl commented 1 year ago

No this is the first time I've ever seen this behavior indeed. So is that a pnp.js issue or something in our code?

bcameron1231 commented 1 year ago

Imo, it's pnpjs, not you. We have some circular references issues (as I linked above) which is real underlying issues with this. and we currently are looking into that. Check the open PR linked above. https://github.com/pnp/pnpjs/issues/2681#issuecomment-1549407900 for this specific issue. We'll hopefully get this all resolved for the next release.

HiltonGiesenow commented 1 year ago

I've taken a bit of a stab at this, I wondered if it would be possible to get some feedback? It's at https://github.com/HiltonGiesenow/pnpjs/tree/Improving-SPFx-Testing-Webpart

I changed the web part name ("Hello World" no longer seemed applicable - I had to create a new project to get this working nicely though. npm run spfx still works though) and my idea is to create something that could be quite comprehensive, with different sections and subsections for the different 'surface areas' of PnP JS. As an example, I've added a Create Web and Delete Web test.

I'm just running the webpart in the workbench - that's probably fine I guess? I've increased the real estate to make it wider for this.

Incidentally, I also included spfx-fast-serve (https://github.com/s-KaiNet/spfx-fast-serve).

Thoughts?

bcameron1231 commented 1 year ago

Thanks for sharing your WIP for SPFx testing! I do have some thoughts (Julie/Patrick may as well).

I'm curious if we could loosely couple these a bit more. While this SPFx solution can be used for testing, it's also what we use for debugging issues in this issues list, but also testing new features. As such, I would like to only import the testing components when needed and leave the Web Part as a vanilla js application where we can continue to do our own debugging. Does that make sense?

Additionally, you introduced some new dependencies that I would like to try and avoid. 1. While fast-serve is really nice (use it in my own development), it's an unnecessary dependency. 2. You also introduced React to the SPFx solution, which I don't think is needed. Could you keep it just Vanilla JS, so we can reduce our dependencies?

In terms of the tests themselves, I think having a button to trigger the test may be cumbersome if we're looking at doing a suite of tests for SPFx. Could we think of a way of just a single button that fires off all of the tests and just outputs the test & result to the page (or just runs on load)?

Lastly, where possible, try and use the settings file (for example, the testWebName).

Thanks!

HiltonGiesenow commented 1 year ago

So overall, here's what I had in mind my side for this web part:

  1. It's a place that's super easy / low friction for a new contributor to get started. I'm thinking about my own experience recently contributing a fix, and finding it confusing to get started and to be able to add a failing test, make a fix, and verify completion.
  2. This is maybe secondary, but is itself quite useful - a place to see usage of the library in action. Docs are important of course, but sometimes a wider scenario can be important, and this kind of sample code would be good.

It sounds like what you / the core team team is doing here is some kind of 3rd item (what you're mentioning in your comment about debugging). Could it not be possible to roll that into option 1? If you're working on an issue, perhaps it needs a failing test first in any case?

As an alternative, maybe we should have two separate SPFx projects - one for the broader testing, and one for your debugging scenarios? Thoughts on this?

Regarding a "master" button to trigger all tests - my thinking with this web part is exactly the opposite. For full end-to-end testing, we should rather have the automated test bank that we have already. This web part, in my mind, is for testing one specific operation or scenario. Another way to think about it maybe is (1) this SPFx webpart for Dev and then (2) the full suite from the command line for coverage and CI/CD. What do you think?

With regards React, relative to 1 & 2 above, while it does of course add a dependency, my thinking here is to meet people where they are - to my experience, React is the predominant mode for SPFx development (I've only ever met one person who does 'native' js for SPFx), but that of course could just be my echo chamber. Do we have access to any kind of stats that could help make this more a informed discussion?

Regarding fast-serve, my thinking here also was two-fold:

  1. As you know, it makes a HUGE difference in the dev-test-fix-repeat cycle, and if we flesh this out more there are probably 100's of tests to add, so if possible and with low impact overall, I'd suggest we use it here.
  2. As this it not an -actual- production system nor is it an actual "sample" solution, I don't see much risk to the community to having it included in this way. My worry here is that a 'sample' can easily become a 'gospel', whereas this is neither a sample nor does fast-serve affect the final code in any way that might influence/confuse people. Thoughts?

Regarding the settings file - makes perfect sense, never thought of that - will update accordingly.

HiltonGiesenow commented 1 year ago

On a totally separate but related note, I'm in two minds about side-effects for these tests. Should they provision and clean up their own dependencies, or simply report if something's missing? As an example, see the "Delete Web" example in the code I've put up. Should it provision the web first? Similarly with the "Create Web" example, should it just leave the web there, or should it delete it afterwards to clean up after itself (side effect free)? What do you think?

bcameron1231 commented 1 year ago

I agree that we can most definitely make the contribution experience easier/clearer. Additionally, we have fairly good test coverage currently (can always be improved) on the core library. Imo, what we are missing in terms of testing is in the realm of the behaviors (using.(...spfx)) themselves that are only executed in the SPFx/SP context. So I do see value in having some tests in the SPFx side of things.

This web part, in my mind, is for testing one specific operation or scenario.

Maybe I'm missing something, but this is how we currently do it. We update the SPFx solution when we need to test a specific operation or scenario that we need to either debug from the issues list, or when we're adding new functionality that we want to test in SPFx. In most cases, I'm actually testing in the node project because it's easier, and only test in the SPFx solution when I have a behavior specific issue I need to try and diagnose.

In regard to dependencies, this is my personal opinion, but I don't see the need to add additional dependencies for fast-serve or React. While React is definitely a commonly used language, it is also not the only language, and we're agnostic of that. I'd rather not introduce a new dependency where it's not needed. This is a library, and I'd prefer that we can focus testing the library itself, without worrying about the additional dependencies that may cause issues in the future while testing the library.

This is maybe secondary, but is itself quite useful - a place to see usage of the library in action. Docs are important of course, but sometimes a wider scenario can be important, and this kind of sample code would be good.

In regard to samples and seeing actual usage, there are quite a bit of samples (linked in our docs) and within the PnP Samples Gallery that showcase the use of PnPjs in real world scenarios. I highly recommend folks to use the Samples Repo for more usage in Web Parts.

Maybe it would make sense to update/extend an existing sample that exists in the PnP Repository to show off the usage of the PnPjs library. https://github.com/pnp/sp-dev-fx-webparts/tree/main/samples/react-pnpjsexplorer It is currently using V2, but it's a nice sample that shows and allows for the capabilities to test and run the library methods. Maybe this is a good place for this kind of work to happen.

On a totally separate but related note, I'm in two minds about side-effects for these tests. Should they provision and clean up their own dependencies, or simply report if something's missing? As an example, see the "Delete Web" example in the code I've put up. Should it provision the web first? Similarly with the "Create Web" example, should it just leave the web there, or should it delete it afterwards to clean up after itself (side effect free)? What do you think?

They should be self-sufficient imo, without need for manual updates to run. Much like our Mocha tests, I'd say it should provision all the assets it needs to test, and with the ability to deprovision as necessary.

HiltonGiesenow commented 1 year ago

We update the SPFx solution when we need to test a specific operation or scenario that we need to either debug from the issues list, or when we're adding new functionality that we want to test in SPFx.

I'm struggling to understand this, because in the main branch, this SFPx web part is practically empty. What happens with the code you write when you're using it? Do you not check that in? Do you remove it before checking in?

Thanks for the links to the Samples repos. I was totally unaware of them which, I'd suggest, is part of the problem. Surely 'samples' for an open source project should be 'in' the project itself rather than a place people might have no idea to look in?

In any case, as I said, the 'samples' part was secondary - my initial suggestion (the reason for opening the issue in the first place) was around testing, and overall, while this conversation has been very insightful, I think we've gotten a little off track - basically, to put things in a nutshell, and to be frank, the project at the moment is highly resistant to new contributors, based on my recent experience. Yes, if I were an existing ongoing contributor, it would be fine, but I'm not so this is my experience new to the project, and I was almost scared off from contributing the fix because of the overhead currently with testing it. What I'm suggesting here overall would -massively- lower the bar for this. Are we at least on the same page with regards to that? If not, let me know and I'll clarify (stopping short now in the interests of brevity)

bcameron1231 commented 1 year ago

I'm struggling to understand this, because in the main branch, this SFPx web part is practically empty. What happens with the code you write when you're using it? Do you not check that in? Do you remove it before checking in?

Yes, these changes aren't checked in. I just stash them and carry them over into the branches I'm working in. My current SPFx solution has code relating to about 4 different issues in the issues list I've been testing. However, most of the debugging and testing is done in the node solution. The SPFx solution helps for SPFx behavior debugging though.

Are we at least on the same page with regards to that?

As someone who had to go through the same thing (learning this project), I know where you are coming from, and I don't disagree we could improve some the guidance around contributing (documentation can always be improved). If you could clarify what you mean by the overhead to testing it? I think we do a pretty good job describing how to do the debugging process. As mentioned previously, the SPFx solution isn't currently broadly announced because it may have it's own issues, and it's why we recommend using the Node solution for debugging and testing. It has auto-watch and you can run most of the library methods much faster without dealing with SPFx transpiling nuances. It's a much simpler path for doing your local debugging/testing (assuming the settings file has been set up correctly).

HiltonGiesenow commented 1 year ago

If you could clarify what you mean by the overhead to testing it?

Sure. I'm putting on kind of two different hats here:

  1. Someone totally new to the project and
  2. Someone who may not have experience with every element that's involved with what's required for setting up testing (Azure AD Application, certificates, etc.)

Maybe as a "2.5" consider also an enterprise developer who has no ability to even create Azure AD applications in their org, and currently has no Developer tenant - they just want to contribute a small fix. For example, my recent PR was adding a single function call - total code addition basically one single word. Now to TEST that change, from the perspective of this new contributor:

  1. The SPFx webpart is not advertised (as @patrick-rodgers mentioned right upfront)
  2. It's empty, so that doesn't seem like it's meant to be used, nor does it include existing code as a guide for what/how to write, so I know neither IF nor HOW to use, presuming I even know it's there
  3. Requiring me to set up an Azure AD app for just a single fix (e.g. in the case of my PR it was literally a single word) is a very high requirement (many enterprise devs I think have still never done this because they don't even have the rights to do so in their own organisation)
  4. Requiring someone to work with certs is always a big ask, IMO, and also something many people will never have done before.
  5. Yes, of course I can do this all in a Developer tenant, but I might not have one of those either, and it's just one more thing to worry about.
  6. None of this is well documented in the project right now. The 'Contributing' section interlinks between sections, but doesn't have a solid flow at all (step 1, step 2, step 3) and makes too many assumptions about the expertise level of an incoming contributor.
  7. I need to install Python, another entire programming language, to be able to test (I haven't done this yet, just per reading https://pnp.github.io/pnpjs/contributing/setup-dev-machine/#setup-your-development-environment). I might also not even be able to do this as an enterprise developer, so now I need to log an IT helpdesk ticket, to be able to contribute to the project.

Again, I'm not talking here about an existing contributor like yourself, or someone who's familiar with all the steps above, but for someone new, asking them go to through what could amount to hours of work to submit a single word fix - in my mind, that's an -extremely- high bar. I know several experienced and talented developers who are too intimidated to contribute to open source projects, and any one of the requirements above could be putting people off, but all seven??

The alternative I'm proposing looks like this:

  1. I find the code with the error
  2. I find some matching testing code in the test web part, and modify it or copy-paste to get a failing test
  3. Run in workbench - super easy to do ('npm install' and 'npm run spfx'), and something I am already familiar with as an SPFx developer
  4. Fix the bug, confirm the test is passing, submit PR, and done

A document on how to do this would be super simple to write, to get the person started.

I haven't looked yet at how the other automated testing is done yet, but perhaps we can even look at a way to combine them, so that the console-based approach and the SPFx approach use the same underlying test execution code, which would make this an even better story - whatever test the contributor writes, for use in SPFx, could then be directly leveraged in a matching automated test easily enough.

looking forward to your thoughts.

bcameron1231 commented 1 year ago

We appreciate your input and would like to address some of the concerns you've raised here to ensure clarity. It is crucial to maintain the quality and stability of this library, considering that it is utilized for over 42 billion requests into SharePoint every month. To achieve this, we must adopt a robust and thorough approach to testing, which includes the ability to execute and debug as many endpoints and scenarios in the library.

It is essential to note that testing solely within the SPFx solution is insufficient for determining the effectiveness of a code fix. This library is used not only for SPFx but also for Node and other environments. Therefore, it is crucial that any implemented fix is verified to work across different platforms. We already have a comprehensive set of tests using Mocha/Chai to validate the internal workings of the library. To contribute to the library, it is a requirement to be able to run and validate these tests, which necessitates setting up a node environment.

Regarding points 1-7, I understand the complexity involved, and we acknowledge the need for improved documentation in this area. However, even with the SPFx approach, it is not a straightforward process. Point 7, specifically, is a prerequisite for any SPFx development, regardless of PnPjs. In order to run an SPFx solution, including testing PnPjs as you suggested, contributors must follow the outlined development environment setup (please note that Python is installed when Gulp is installed for you. So you may have already had it.).

While not all fixes require it, many aspects of the library rely on the ability to have administrative privileges and grant necessary permissions to execute tests and methods. For example, Graph calls, Taxonomy calls, and high rights mask calls. Again, to ensure the stability of the library, contributors must be able to run the complete suite of tests using Mocha/Chai.

While I understand the desire to lower the barrier to entry for contributors, it is essential that we are able to thoroughly test and run the full range of functionality within the PnPjs core library. Without the necessary elements such as admin rights, a node environment, and the ability to test in SPFx, we cannot guarantee that even a small code change will not cause issues elsewhere. As a community-driven project, we strive to uphold a certain standard due to the widespread usage of this library.

I hope this explanation provides a clearer understanding of the requirements and considerations involved.

juliemturner commented 1 year ago

Thanks @bcameron1231, that was really well said.

I can totally sympathize with the high bar that contributing to this library requires. It took me a good year and deep work on V3 to feel like I fully understand how it works and the elegance of the underlying codebase to provide a very lightweight but flexible SDK. As @bcameron1231 points out this library is used by literally 10s of thousands of tenants and the weight of the responsibility to not mess it up weighs heavily on all of us maintainers.

While it does take a bit of setup, once done all of that essentially fades into the background. Also, while I understand that the SharePoint Framework has obfuscated some of the technical underpinnings of working with the APIs, this library is not only for the SharePoint Framework. I'm super happy that it can support those that use it but being a contributor to the library really requires an understanding of all the surfaces we support so that, as said above we can provide a high-quality interface on all surfaces we support. I would also say that enterprise developer or not, if you are developing primarily in your production environment that's probably not the best idea. Dev Tenants are free and very easy to obtain, I would encourage everyone to get one. If it's your job to extend the Microsoft 365 platform, then the IT department should not only understand but want you to use the proper tools to do that job in the best way possible.

All of this said, your feedback about the lack of clarity of each step of the contribution setup process is valid. We did make an effort with V3 to enhance that documentation, but we can always do better and for V4 we will tackle documentation once again and try to do better.

HiltonGiesenow commented 1 year ago

I'm realising now I oversimplified :-(.

I was certainly NOT suggesting we don't have the comprehensive tests. I was suggesting that a new contributor not necessarily need to contribute a fix to that extent, if they're unable to get all the other bits up and running - it's something that a more 'core' team member could do, once at least the minimum bar of (a) a fix and (b) at least an SPFx test is included.

Hoping that clarifies better?

HiltonGiesenow commented 1 year ago

In any case, thanks @bcameron1231 and @juliemturner , it's been a very enlightening discussion and I very much appreciate the huge amount of time you both and @patrick-rodgers put into this incredibly important project.

Circling back to my original suggestion though, I can see this clearly isn't something that the team thinks would be either valuable or even desirable, so perhaps let's just close it out, I'm hesitant to waste any more of any of our time on this sideline discussion when I'm sure the attention could be better spent elsewhere.

BR,

Hilton

github-actions[bot] commented 1 year ago

This issue is locked for inactivity or age. If you have a related issue please open a new issue and reference this one. Closed issues are not tracked.