#5341 code analysis documentation #5882
Conversation
…eded to run the cypress tests
✅ Deploy Preview for plone-components canceled.
|
✅ Deploy Preview for volto ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@stevepiercy before passing to tech review I'm sure you will find a bunch of wording changes so after you make all of the suggestions and you give the go-ahead I will add other core devs to have a look. |
There was a problem hiding this comment.
I pushed a commit for cleaning up acceptance-tests.md.
I have to admit, this is the first time I have ever run Cypress locally, and I'm amazed. It might actually encouraged me to contribute to Volto core, beside its documentation. @ichim-david is my hero.
I cannot emphasize enough the importance that this contribution has to future contributions to Volto.
I still need to review the other files, but I have to say so far, this single contribution may do more than any other contribution in recent history to encourage more contributions.
"If it ain't documented, it's broken" reigns true.
@stevepiercy this reply is awesome, thank you for the appreciation, I appreciate it deeply :). I will be one of your tag team partners, I will write something that will get us to the 80% done and you will get it to that final 20% to reach the final line. It's hard to write and write good but knowing that I have someone like you that can iron things out makes it easier for me to want to contribute and help. |
|
@ichim-david I think we don't need to wait for my English grammar and documentation structure organization. Let's get everyone who should be involved in reviews participating now, as everyone has different schedules and availability. It will be an iterative process, but it will be much better than what we have now. For the acceptance tests, are there any other use cases that should be included? To be honest, I'm overwhelmed with how we can use Cypress, and I have no idea how to best use it. |
| ## How to run acceptance tests locally (during development) | ||
|
|
||
| When writing new acceptance tests, you usually want to minimize the time it takes to run the tests, while also being able to debug or inspect what's going on. | ||
|
|
||
| Being able to restart individual components also comes in handy. | ||
| It's recommended to start three individual terminal sessions, one each for running the Plone backend, the Volto frontend, and the acceptance tests. | ||
| All sessions should start from the `packages/volto` directory. |
There was a problem hiding this comment.
This is not true. You can run them from the root too, they have similar proxy commands to packages/volto
There was a problem hiding this comment.
Just because you can do something doesn't mean you should do it. We are documenting to beginners how they can run the acceptance testing and if they really care they can see that the commands expand to the volto package.
Within the volto package you have everything you need from the news folder to lint to run to test and although power users can do whatever they want from volto repo, well that's a power user move :).
Although I didn't write it this way exactly I agree with Steve's directive to assume all commands for volto core run from packages/volto folder.
EDIT:
Another reason to use the workflow of go inside packages/volto folder is that we can show the commands using only pnpm command which would have the equivalent also in projects and 17.x.x and 16.x.x branch by swapping pnpm for yarn instead of going over
pnpm --filter which doesn't have equivalent in the order, still maintain and I would even say current versions until 18 is released.
There was a problem hiding this comment.
I actually didn't recommend one place over another. It was my interpretation of the link that @ichim-david had originally used to refer to this location. This brings up something we need to discuss and decide.
There are quite a lot of differences between the two Makefiles. It looks like the primary Makefile is at the repo root, based in its inclusion of documentation commands and reading files from the repo root. It also appears that the one in packages/volto has not been kept up to date with that at the repo root, based on the number of updates to each. For example, I would not run make build from the package, but I would from the root. Here's another significant difference where some commands fail when spaces are in the realpath, which is something that often bites first-timers on their bum.
- CHECKOUT_BASENAME=$(shell basename $(shell realpath ./))
+ CHECKOUT_BASENAME="$(shell basename $(shell realpath ./))"
- CHECKOUT_TMP_ABS=$(shell realpath $(CHECKOUT_TMP))
+ CHECKOUT_TMP_ABS="$(shell realpath $(CHECKOUT_TMP))"After looking at these differences as they are today, I think that we should recommend all commands should be run from the root of the repo. We can also suggest a tip that they may be run from the packages/volto directory, but there is no guarantee that they will work, such as make docs-*. That is, unless and until, we decide that the Makefile in the packages/volto directory is maintained for full parity with that at the root of the repo.
@sneridagh is that interpretation of Makefiles correct?
There was a problem hiding this comment.
This is not true. You can run them from the root too, they have similar proxy commands to
packages/volto
Not quite, https://github.com/plone/volto/blob/main/package.json#L14-L23 although yes in the root you have commands for lint, prettier and stylelint and the :fix options they run for all of the apps and packages defined in the monorepo while the commands from packages/volto only run for the volto core package.
Yes, you can also manually run the filter commands from the root but then you lose the convenience of simply writing pnpm command and getting the right outcome.
We can enhance the docs with another section for the apps and other packages from the volto umbrella but now we should ensure that we have the docs for how to contribute to Volto core in my opinion.
docs/source/contributing/linting.md
Outdated
| it will be easier to run the commands only for Volto core within the Volto core folder: | ||
|
|
||
| ```shell | ||
| cd packages/volto |
There was a problem hiding this comment.
I think we should mention the --filter @plone/volto command from pnpm here (and maybe other places, whenever suitable).
|
I've completed my review. @sneridagh @ichim-david please take another look at the whole thing, and check the preview build, too. |
|
@sneridagh @stevepiercy It would be too weird to accept my own pull request :) but it feels like something so much better after all of the extra formatting and references done by Steve. I've checked it and from my point of view it's a merge, I'm waiting to see if Victor has anything else that wants to be changed otherwise we could merge anytime we feel like it. |
There was a problem hiding this comment.
@ichim-david @stevepiercy this is awesome, please allow me to add myself to the praise chain. 🙏
Let's merge it right away.
Fixes #5341 code analysis after some much-needed magic from @stevepiercy :)