Skip to content

Extend test suite to run more tests also in pre-commit#3702

Open
terhorstd wants to merge 12 commits intonest:mainfrom
terhorstd:improve-pre-commit-checks
Open

Extend test suite to run more tests also in pre-commit#3702
terhorstd wants to merge 12 commits intonest:mainfrom
terhorstd:improve-pre-commit-checks

Conversation

@terhorstd
Copy link
Copy Markdown
Contributor

@terhorstd terhorstd commented Dec 5, 2025

The test framework uses some outdated versions that start to become difficult with newer python versions. The main change in this PR is the update of some pyproject.toml sections and the .pre-commit-config.yaml. New checks are added to improve developer-local checks as well as the CI side.

In more detail:

  • The mypy.ini was translated to pyproject.toml to have config in one place
  • More pylint errors are silenced for now to have them being fixed in a separate PR.
  • nest-server code was slightly refactored to meet absolute minimum quality requirements.
  • some files with implicit relative import require explicit from . import connect_test_base to be checked. (which is subsequently resorted by isort)
  • Some test implementation type-checking suffers from untyped pynest API, thus requiring explicit annotations like n_events: int = spike_recorder.get("n_events")
  • mpi-tests can only be checked if directory-names are valid python module names, therefore the rename of mpi/2/*.pympi/nproc2/*.py
  • Some updates caused a re-formatting of cpp file indention levels. (change "files changed" settings to hide white-space changes to de-clutter the review)
  • minor changes to shell-scripts as suggested by shellcheck

@terhorstd terhorstd added S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Dec 5, 2025
@terhorstd terhorstd requested review from gtrensch and heplesser and removed request for gtrensch and heplesser December 5, 2025 11:52
Copy link
Copy Markdown
Contributor

@gtrensch gtrensch left a comment

Choose a reason for hiding this comment

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

LGTM! 👍 Approved.

Copy link
Copy Markdown
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

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

@terhorstd Thanks for this PR. Unfortunately, it comes with several changes that I believe hurt code readability, that I do not understand and that break the testsuite.

Concerning code formatting for C++, I think the changes to for-loop header indentation reduces readability. Even worse is the the line break after :: in many method declarations. Here, we need to stick to the old style.

I also do not understand why all tests suddenly are converted to importable Python packages. As far as I understand the Pytest documentation, when keeping tests outside the installable package as we do, tests should not be packages (https://docs.pytest.org/en/7.1.x/explanation/goodpractices.html#tests-outside-application-code).

All tests currently fail because connect_test_base.py is not found when tests try to import it.

Comment thread .pre-commit-config.yaml
Comment thread bin/nest-server-mpi Outdated
Comment thread bin/nest-server-mpi
comm.gather(nest.serialize_data(response), root=0)
def main():
opt = docopt(__doc__)
if rank == 0:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Here, rank is a global variable, is it not? Can we avoid that, maybe by restructuring the code?

Comment thread bin/nest-server-mpi
nest.server.run_mpi_app(host=opt.get("--host", HOST), port=opt.get("--port", PORT))

else:
logger.info(f"==> Starting NEST Server Worker on rank {rank}")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The else block is very large. Could one restructure the code to avoid such a long else block? Maybe return at the end of the if block?

Comment thread nestkernel/position.h
template < int D, class T >
Position< D, T >::operator std::string() const
Position< D, T >::
operator std::string() const
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This reformatting looks very strange to me, significantly reducing code readability. Can we keep the old form and get the formatter to understand that this should be one line? Breaking on :: seems really weird to me.

Comment on lines +943 to +944
i != thread_local_wfr_nodes.end();
++i )
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I do not like this change. I think the loop is more readable if all parts are aligned under each other as in the old version here. Can we not keep the old rules?

@terhorstd terhorstd moved this from To do to In progress in Build system and CI Jan 26, 2026
Co-authored-by: Hans Ekkehard Plesser <hans.ekkehard.plesser@nmbu.no>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation.

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

3 participants