Skip to content

Conversation

@klaernie
Copy link
Member

@klaernie klaernie commented Feb 2, 2025

After looking at the recent regression I think it is a good idea to run tests one every change. This way we should no longer accidentally publish a release that does not work for our users.

The tests done are twofold:

  • the first portion runs the same kind of test we use in run-tests.sh: using the default python installation, install the local package and then run ec --version to ensure a working binary has become available. This is matrixed over the platforms GitHub Actions provides: ubuntu both x64 and arm64, MacOS 13 (the last Intel variant) and 15 (arm64 M1), and lastly x64 Windows (sadly no ARM support there yet).
  • second portion runs make test on an Ubuntu 24.04 runner, verifying that our backwards compatibility to all python releases is intact.

@mmicu
Copy link
Member

mmicu commented Feb 7, 2025

Thanks for your work here, really useful. I will try to review the changes next week.

@klaernie
Copy link
Member Author

I rebased this branch on top of the next-release branch

- name: test that editorconfig-checker works by letting it output it's version
run: |
ec --version
Copy link

Choose a reason for hiding this comment

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

This is also a kinda weak test, as there's only the exit code that can make the step, thus the workflow, fail.

I remember that for Megalinter, the published binaries for a go tool (probably revive), didn't actually have a version information (maybe limited to the ones released through Docker images). The workaround needed was to build ourselves, like

https://github.com/oxsecurity/megalinter/blob/73c05baa4078059527411e1784a0db4f8b9df94b/megalinter/descriptors/go.megalinter-descriptor.yml#L86-L101

Copy link
Member Author

Choose a reason for hiding this comment

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

I was intentionally mirroring the make test - if we decide to improve this we should do it in both places at the same time.

run-tests.sh Outdated
if [ -n "$TEST_PY_VERSION" ]; then
PY_VERSIONS+=("$TEST_PY_VERSION")
else
PY_VERSIONS+=("2.7")
Copy link

Choose a reason for hiding this comment

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

It still runs on Python 2?

Copy link
Member Author

Choose a reason for hiding this comment

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

The docker tests prove that it does - after all it is just a thin wrapper ;)

run-tests.sh Outdated
Comment on lines 10 to 11
PY_VERSIONS+=("3.7")
PY_VERSIONS+=("3.8")
Copy link

Choose a reason for hiding this comment

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

These two are EOL. Do you still support them?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would leave that decision up to @mmicu - I'm fine with keeping EOLed versions as long as there is no technical reason breaking them.
After all editorconfig-checkers place is of a supporting role, maybe helping people slowly transition to newer python versions, and when the only weight for us is some minutes spent running the actions, I do not consider this a cost worth saving. Of cause this argument goes out of the window as soon as we are blocked by a problem that only occurs on EOL versions.

this avoids the need to replace content in the Dockerfile using sed and
makes it possible to run a quick test using `make quicktest` (using the
defaults for the build arguments specified in the Dockerfile)
This reduces the test runtime from about 5min to 3min on my machine.
this tests in two ways:
- install the package locally in each major os/architecture combination
  supported by GitHub Actions, then run `editorconfig-checker --version`
  like `run-tests.sh` does
- running `run-tests.sh` once in an ubuntu machines

The workflow is triggered both by pushes to any of the branches and for
each pull request.
@mmicu mmicu merged commit 52415ff into editorconfig-checker:main Aug 14, 2025
6 checks passed
@mmicu
Copy link
Member

mmicu commented Aug 14, 2025

@klaernie, thanks for your contribution! Your changes will make our release process much smoother.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants