Modernize, remove outdated code, add code-analysis checks #168
Conversation
In coredev, when you run `bin/test -u`, all is well.
When you run `bin/test -s plone.dexterity`, with or without `-u`, one test fails:
```
Error in test testAddContentToContainer_preserves_existing_id (plone.dexterity.tests.test_utils.TestUtils)
Traceback (most recent call last):
File "/Users/maurits/.pyenv/versions/3.10.4/lib/python3.10/unittest/case.py", line 59, in testPartExecutor
yield
File "/Users/maurits/.pyenv/versions/3.10.4/lib/python3.10/unittest/case.py", line 591, in run
self._callTestMethod(testMethod)
File "/Users/maurits/.pyenv/versions/3.10.4/lib/python3.10/unittest/case.py", line 549, in _callTestMethod
method()
File "/Users/maurits/community/plone-coredev/6.0/src/plone.dexterity/plone/dexterity/tests/test_utils.py", line 83, in testAddContentToContainer_preserves_existing_id
item = addContentToContainer(container, item, checkConstraints=False)
File "/Users/maurits/community/plone-coredev/6.0/src/plone.dexterity/plone/dexterity/utils.py", line 175, in addContentToContainer
name = INameChooser(container).chooseName(name, object)
File "/Users/maurits/community/plone-coredev/6.0/src/plone.app.content/plone/app/content/namechooser.py", line 55, in chooseName
return self._findUniqueName(name, obj)
File "/Users/maurits/community/plone-coredev/6.0/src/plone.app.content/plone/app/content/namechooser.py", line 64, in _findUniqueName
if not check_id(name, required=1):
File "/Users/maurits/community/plone-coredev/6.0/src/plone.app.content/plone/app/content/namechooser.py", line 104, in do_Plone_check
return check_id(obj, newid, required=required, contained_by=parent)
File "/Users/maurits/community/plone-coredev/6.0/src/plone.base/src/plone/base/utils.py", line 412, in check_id
result = _check_for_collision(contained_by, id, **kwargs)
File "/Users/maurits/community/plone-coredev/6.0/src/plone.base/src/plone/base/utils.py", line 476, in _check_for_collision
if portal and cid in portal.contentIds():
AttributeError: 'Dummy' object has no attribute 'contentIds'
```
So the portal here is a Dummy from our tests. No idea why there is a difference in how we run the tests, especially without layers.
Anyway, fixed by adding a `contentIds` method to our Dummy class, returning an empty list.
This uses the plone/code-quality docker image. Really fast after the first run. On Mac: `brew install --cask docker`, then start Docker Desktop, or have it start always in the background or something. I made it easy: you can only do `make format` and `make lint`. There is only one gh-actions job, which runs each linter in a separate step. I did not want to create a pre-commit hook, but you could.
'make lint' now finishes without error.
Removed the conditional code. Removed `bbb.py`, which was only used internally.
Always use our own interface.
|
@mauritsvanrees thanks for creating this Pull Request and helping to improve Plone! TL;DR: Finish pushing changes, pass all other checks, then paste a comment: To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically. Happy hacking! |
|
@jenkins-plone-org please run jobs |
|
Why did you prefer to run one check at a time? My approach was to run all of them to:
|
Your approach spins up five VMs, which each checkout the source code and get the docker image. The checks in this PR took 20 seconds. |
|
I still don't understand the advantage of running code-analysis in a container. It's faster? On the first run it sure takes longer. Wouldn't creating a venv solve the problem? |
The idea of having a GitHub action that does that was to have sane defaults and versions for all tools, something that is reproducible easily. Having the action to use a pre-built Docker image speeds up everything, reducing the time to a few seconds. Now, locally, we decided to go with the Docker image to avoid unpleasant surprises when it comes to versions (and yes, that happened a lot). |
On my laptop A venv works too. You could add a In contrast, with the Docker approach, you download the image once, and then use it in all repositories. There is one version number (1.0.1) for the used image in There are multiple ways to tackle this, with pros and cons, and I don't think we have the best solution yet. I still feel like I am exploring possibilities. Some of my goals:
|
@jensens I saw your closed PR #160 again after I started this. I took over a few ideas, but probably not all. Some room for improvement in a later PR, or if you want you can edit this one.
@ericof I added the
plone/code-analysisaction/docker here, in a bit trimmed down form. I only havemake formatandmake lint, and only one GitHub action job which runs all checks after each other.