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

Test fixture root is not correct for some tests #521

Open
jaytaph opened this issue Jul 21, 2024 · 5 comments
Open

Test fixture root is not correct for some tests #521

jaytaph opened this issue Jul 21, 2024 · 5 comments
Labels
bug Something isn't working good first issue Good for newcomers tests Anything related to tests

Comments

@jaytaph
Copy link
Member

jaytaph commented Jul 21, 2024

When running make test, the system will run ./src/bin/parser-test.rs. This tries to find the tests in FIXTURE_ROOT, but they are not present there. However, the tests in the gosub_html5 crate, do find these tests at the correct position, mainly because the FIXTURE_ROOT is a relative path.

We somehow must make this more consistent so we can run both make test, and have the CI pass

@jaytaph jaytaph added bug Something isn't working good first issue Good for newcomers tests Anything related to tests labels Aug 28, 2024
@aemreaydin
Copy link
Contributor

Hello everyone!
First time here, I would like to work on this to get familiar with the project.

I think there are a couple ways this could be done reliably that I can think of.

  1. Change FIXTURE_ROOT variable to use env!("CARGO_MANIFEST_DIR") and concat the rest of the path
  2. Find base project root (possibly the Cargo.lock) and use it as the base path and generate the FIXTURE_ROOT from there.

@jaytaph
Copy link
Member Author

jaytaph commented Oct 19, 2024

Both options seem reasonable. I would opt for the least amount of change for the enduser (so option 2),.. but having option 1 as a backup (so you can override if the system cannot find it for whatever reason) might be nice too.. I just don't know if that is a lot of work or not..

@aemreaydin
Copy link
Contributor

I'm just able to respond as I was away last week. Honestly the simplest change is the first one as it is basically changing the FIXTURE_ROOT variable.
Second one would be more robust way to do it but if we're not using it anywhere else I don't see the need to add the functionality.

aemreaydin added a commit to aemreaydin/gosub-engine that referenced this issue Oct 28, 2024
aemreaydin added a commit to aemreaydin/gosub-engine that referenced this issue Oct 28, 2024
@aemreaydin
Copy link
Contributor

I have created a PR with the second way but figured we should discuss the second way as well.
There is a crate called project_root that literally does that and it's the simplest crate ever that we can implement ourselves.
Project Root Crate
Using this function would be a more general approach imo because it goes straight for the base directory, checking Cargo.lock

jaytaph added a commit that referenced this issue Oct 31, 2024
@aemreaydin
Copy link
Contributor

I was reading the docs and I saw in the binaries docs parser-test, this issue is still listed. Should I remove it in this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers tests Anything related to tests
Projects
Status: 📝 Todo
Development

No branches or pull requests

2 participants