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

Improvements to runtests.py + use github actions as CI #140

Merged
merged 9 commits into from
Jan 28, 2024

Conversation

bmispelon
Copy link
Contributor

Hi,

While looking at issue #136 I ended up running into issues trying to run the test suite to make sure my changes hadn't broken anything. One thing led to another and I found myself tweaking the runtests.py bit by bit, and eventually configuring github actions to run.

I figured I would send this back upstream. If the maintainer team is not interested, feel free to close this.

I've split the PR into (mostly) independent commits that can be reviewed/merged one at a time if it makes things easier.

For a bit of context behind my contribution, we use trac-github on our Trac instance and I've recently started upgrading our old version, running into compatibility issues along the way (as expected). Eventually I'd like to get to Trac 1.6 and Python 3 so I would probably be submitting more PR your way if that's ok (hence why I left a commented out github action for python 3 support)

This helps running the tests on machines where git
is configured with a different defaut branch name.
The Python documentation says not to do that, and the pipe
doesn't seem to be used here anyway.
This makes the code a tiny bit more readable, and
could make mocking/stubbing the actual git respository
easier down the line.
Some git configurations (like mine, as it turns out) don't
include that directory when using git init.
@bmispelon
Copy link
Contributor Author

@neverpanic What do you think? Is this kind of PR something you're interested in?

Copy link
Member

@neverpanic neverpanic left a comment

Choose a reason for hiding this comment

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

Looks good to me and works, improves on the status quo. I'm merging this, thank you very much for the contribution.

COVERAGE = options.with_coverage
SHOW_LOG = options.with_trac_log

if options.virtualenv:
Copy link
Member

Choose a reason for hiding this comment

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

I'm not opposed to this, but I'm wondering if the better solution isn't to run the runtests.py script in the virtualenv itself, where $PATH would hopefully already include the correct instances of trac-admin, tracd, and coverage?

The test script itself already has quite a few dependencies and does import parts of trac, so running it outside of a virtualenv is probably not a very good idea anyway.

Maybe I'm missing a use case that you had in mind?

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind, I noticed that --with-coverage does not work when run in a virtualenv, likely because coverage run --append --branch --source=tracext.github TRACD_BIN doesn't work unless TRACD_BIN is an absolute path.

We could change that to look up tracd in $PATH in startTracd, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was juggling with multiple virtualenvs and instead of activating them I'd call the tests with path/to/venv/bin/python runtests.py which didn't seem to detect the correct path for tracd and trac-admin, hence why I added the flag.

It's very possible there's a better solution to this, but this is the one I came up with 😅

@neverpanic neverpanic merged commit 6db10e4 into trac-hacks:master Jan 28, 2024
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.

2 participants