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

Add README section to run the tests #1144

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SqAtx
Copy link
Contributor

@SqAtx SqAtx commented Sep 21, 2024

Describe a simple way to get the tests running. This should help clear the confusion around the ModuleNotFoundError: No module named 'GTG.core.info' type of errors you get when just running pytest (see #1141).

Adapted from the explanation at #1036

We can close #1141 and #1142

@SqAtx SqAtx requested a review from picsel2 September 21, 2024 21:53
@Neui
Copy link
Contributor

Neui commented Sep 21, 2024

I think the chosen text puts a bit too much on the "why" rather than "how" to run it in a README file, which should be straight to the point.
Additionally, I don't think referencing the GitHub Actions workflow is useful if we already show the commands to use anyway here.
(I think there is a mistake regarding build directory.)
My suggestion:

### Running the tests

Tests are run using `pytest`.

Currently, gtg doesn't support testing in-place,
but you can copy the required files into your source tree for local development:
```sh
meson setup ./.build  # Generate required files
cp .build/GTG/core/info.py GTG/core/  # Copy required files
```

Then you can run the tests with `pytest` in the repository root.

Also:

  1. What about using .local_build created by the launch.sh script instead?
  2. Using ln -s to create that link may also be useful so the file "auto-updates".
  3. We should include how to run tests for the built version (I think it's ninja -C .local_build test or something, I would need to look into that again)

@diegogangl
Copy link
Contributor

I agree with @Neui 's remarks.

What about using .local_build created by the launch.sh script instead?

Not so sure about this. I think it's better to keep builds-to-run and builds-to-test separate, to avoid any mixups

@SqAtx SqAtx marked this pull request as draft November 8, 2024 23:01
@SqAtx
Copy link
Contributor Author

SqAtx commented Nov 9, 2024

I think the chosen text puts a bit too much on the "why" rather than "how" to run it in a README file, which should be straight to the point.

Fair point. Figured I'd put it here to remind myself of the "why" but you're probably right - there may be a place for such documentation but that place isn't here.

Additionally, I don't think referencing the GitHub Actions workflow is useful if we already show the commands to use anyway here.

The thinking behind it was that if they ever drift apart (i.e. we update the workflow but not the README), the README will still point to an example of a setup that works. That said, the workflow actually does a lot more than this simple setup, so it might not be that relevant.

We should include how to run tests for the built version (I think it's ninja -C .local_build test or something, I would need to look into that again)

Come to think of it, maybe that's what the GHA workflow should be running, even. I'm not too fussed about that right now, just trying to lower the barrier to entry.

following one-time step:
```sh
meson setup ./build
cd GTG/core/ && ln -s ../../.build/GTG/core/info.py .
Copy link
Contributor Author

@SqAtx SqAtx Nov 9, 2024

Choose a reason for hiding this comment

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

This isn't pretty, but my first idea didn't work:

$ ln -s .build/GTG/core/info.py GTG/core/
$ ls -l GTG/core/info.py
lrwxrwxrwx 1 kevin kevin 23 Nov  8 21:10 GTG/core/info.py -> .build/GTG/core/info.py

@SqAtx SqAtx marked this pull request as ready for review November 9, 2024 05:15
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