Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Testing matrix #256

Merged
merged 9 commits into from
Feb 21, 2025
Merged

Testing matrix #256

merged 9 commits into from
Feb 21, 2025

Conversation

gforcada
Copy link
Member

@gforcada gforcada commented Feb 11, 2025

Part of #234

So far:

  • split the gigantic tox.ini.j2 into more sizeable chunks
  • tox environments' names are generated correctly
  • tox picks the correct python version
  • multiple constraints files are specified so that each environment is using the correct one
  • you can override the specific testing matrix on .meta.toml (if we had not enough configuration options 😓 )
  • generate the right GitHub configuration 🎉 with proper names 😆
  • documentation!
  • GitLab equivalent changes to the GitHub ones

@1letter
Copy link

1letter commented Feb 12, 2025

first quick test give me an error:


$ tox -e init
init: install_deps> python -I -m pip install mxdev
init: commands[0]> mxdev -c mx.ini
###############################################################################
# Load configuration
Can not parse override: 
###############################################################################
# Read infiles
Read [r]: requirements.txt
Read [c]: constraints.txt
Read [c]: https://dist.plone.org/release/6.1-latest/constraints.txt
###############################################################################
# No sources configured!
###############################################################################
# Write outfiles
Write [c]: constraints-mxdev.txt
Write [r]: requirements-mxdev.txt
🎂 You are now ready for: pip install -r requirements-mxdev.txt
   (path to pip may vary dependent on your installation method)
init: commands[1]> echo 'Initial setup for mxdev'
init: failed with echo is not allowed, use allowlist_externals to allow it
  init: FAIL code 1 (2.57 seconds)
  evaluation failed :( (2.69 seconds)

@gforcada
Copy link
Member Author

@1letter oh, sorry, it was not meant to be tested still, but indeed, there is some polishing needed. Thanks for the heads up. After the next meeting I will fix it

@gforcada gforcada force-pushed the testing-matrix branch 6 times, most recently from 20c665b to cb0f066 Compare February 15, 2025 19:31
@gforcada gforcada requested review from 1letter, davisagli, stevepiercy and mauritsvanrees and removed request for 1letter February 15, 2025 19:34
Copy link
Contributor

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions and a question. I didn't bother to submit multiple suggestions for a single sentence or concept. Narrative docs should follow the one sentence per line guideline of Plone Documentation, and not break up concepts for semantic line breaks or PEP8 line length. Semantic line breaks are harmful, and documentation is not code.

@@ -0,0 +1 @@
Allow to define multiple plone and python versions in tox @gforcada
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Allow to define multiple plone and python versions in tox @gforcada
Support combinations of multiple versions of Plone and Python in tox. @gforcada

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though it is not only tox, it's being used in GHA and GitLab as well, that's why I kept it generic

@mauritsvanrees
Copy link
Member

@gforcada I run into an error trying this.

I ran this on a branch of collective.faq where I am working on porting it to Plone 6. See collective/collective.faq#26. The current tests (without this new testing matrix support) fail while trying to run buildout on a config that does not exist. :-) So the testing matrix support could help there.

But when I run config-package on it, I get a traceback:

$ ./venv/bin/config-package --branch current /Users/maurits/zest/irceline6/src/collective.faq
*** towncrier: If you want to use Towncrier, you have to create a 'news/' folder manually.

Traceback (most recent call last):
  File "/Users/maurits/community/plone-coredev/6.2/src/meta/./venv/bin/config-package", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/Users/maurits/community/plone-coredev/6.2/src/meta/src/plone/meta/config_package.py", line 747, in main
    package.configure()
  File "/Users/maurits/community/plone-coredev/6.2/src/meta/src/plone/meta/config_package.py", line 722, in configure
    files = method()
            ^^^^^^^^
  File "/Users/maurits/community/plone-coredev/6.2/src/meta/src/plone/meta/config_package.py", line 388, in tox
    options.update(self._handle_constraints_files(options))
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/maurits/community/plone-coredev/6.2/src/meta/src/plone/meta/config_package.py", line 400, in _handle_constraints_files
    plone_versions = list(test_matrix.keys())
                          ^^^^^^^^^^^^^^^^
AttributeError: 'str' object has no attribute 'keys'

I did not yet edit .meta.toml to do anything with a test matrix yet. In fact, I have now basically emptied it from my previous manual attempt.

@mauritsvanrees
Copy link
Member

@gforcada The problem is that if test_matrix has not been adapted in .meta.toml, the options dict will contain a default value "test_matrix" = "".

I have this change locally and then it works:

             test_matrix = options.get("test_matrix", TOX_TEST_MATRIX)
+            if not isinstance(test_matrix, dict):
+                test_matrix = TOX_TEST_MATRIX
             plone_versions = list(test_matrix.keys())

I am not sure this is the best way, but it helps. And all jobs on my PR are green, for example this one. So looking good!

@mauritsvanrees
Copy link
Member

Ah, but the job called "6.0 on py3.13" actually has Products.CMFPlone 6.1.0. The other jobs are correct.

tox runs a pip install with -c/home/runner/work/collective.faq/collective.faq/.tox/py313-plone60/constraints.txt

Locally, .tox/py313-plone60/constraints.txt only has one line: zope.testrunner.
Ah, but locally .tox/py39-plone60/constraints.txt also has only this one line. There, just like on GitHub Actions, the py39 one is using Plone 6.0, and py313 is using 6.1.

Strange. There is some magic in tox that is not yet obvious to me.

@mauritsvanrees
Copy link
Member

  • Locally py39-plone60 has plone.staticresources 2.2.3, which should be 2.1.17.
  • Locally py313-plone60 has plone.staticresources 2.2.2, which should be 2.2.0.

I don't know why those versions are higher than they should be.

Tox configuration was getting too big to handle in a single file.

While doing that, take the time to reduce the duplication between the
test sections, as the differences are only on the requirements and the
commands, but everything else is the same.
@gforcada gforcada marked this pull request as ready for review February 21, 2025 12:43
@gforcada gforcada force-pushed the testing-matrix branch 2 times, most recently from ea22067 to 5e81050 Compare February 21, 2025 12:55
@gforcada
Copy link
Member Author

@stevepiercy fixes all applied, hopefully this time is all good from your side 🤞🏾

@mauritsvanrees the AttributeError should be fixed, very weird the constraints problems, let me double check as well

@gforcada
Copy link
Member Author

@mauritsvanrees

Somehow the .tox/ENV/constraints.txt only has zope.testrunner and no the complete Plone 6.0/6.1 constraints file ❗

In .tox/test/constraints.txt though, the complete constraints file is there 🤔

@gforcada
Copy link
Member Author

I found the problem:

The [testenv] section in tox.ini had a typo:

-deps = {[test_runner]deps}
+deps = {[base]deps}

I'm fixing it right away

Copy link
Contributor

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few grammar and proper noun tweaks, and a few semantic line break habits to break, and it's good to go.

This allows running any combination of Plone and Python versions that
are configured on the repository itself, or the defaults provided by
`plone.meta` itself.
As the default `[testenv]` is taken over for the generated environments,
we need to set the settings which were defined there in the environments
that need it.
Otherwise, the testing matrix is pointless!

Add a new setting for the other tox environments that also need a
constraints file, but not multiple.
Like in Jenkins, for each plone version, run the tests against the
python versions upper and lower bounds.
Copy link
Contributor

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs LGTM. Needs a technical review from someone more knowledgeable. Thank you!

Copy link
Member

@mauritsvanrees mauritsvanrees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, with your last fix it works on my collective.faq PR.
This is very helpful. Many thanks!

@mauritsvanrees mauritsvanrees merged commit f07713e into 2.x Feb 21, 2025
1 check passed
@mauritsvanrees mauritsvanrees deleted the testing-matrix branch February 21, 2025 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants