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

Simplify project root detection in tutorial test #246

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

Conversation

DavidWHelsing
Copy link
Contributor

The project source code detection in the tutorial test was using gix to detect root of the rust project by looking for the .git directory.
As far as I can tell this is unnecessary since rust tests are always executed from the project root. So the working path should already be the correct directory.
Even if the tests were somehow manually executed from a different path I don't think the existing code would have worked since it was joining current working directory with crate::parent_directory!() which would be the relative path of the directory in which our code file is located. That would be incorrect for any other working directory.

This assumption might not work in a cargo workspace but I think that this code would already not work in a workspace and this project isn't using a workspace right now.

Simplifying this code also allows us to remove the dependency on on gix. Since this is a dev dependancy change there should be no need for a version bump

@DavidWHelsing DavidWHelsing self-assigned this May 9, 2024
dbg!(git_root);
let project_root = std::env::current_dir().expect("Failed to get working directory");
let project_root_str = project_root.to_str().expect("Failed to convert to str");
dbg!(project_root_str);
Copy link
Contributor

Choose a reason for hiding this comment

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

can we remove this?

.to_str()
.expect("failed to convert path git root to string");
dbg!(git_root);
let project_root = std::env::current_dir().expect("Failed to get working directory");
Copy link
Contributor

Choose a reason for hiding this comment

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

this is likely going to break

e.g. the gix code was added for a reason by @qsantos iirc

Copy link
Contributor

Choose a reason for hiding this comment

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

but: if the tests pass, im happy to merge

Copy link
Contributor

Choose a reason for hiding this comment

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

There was no detection for the actual root of the project. Maybe this was added since, so manually navigating to the parent directory is not needed anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is using current working directory. So that's the directory from which it's executed.
Invoking this test using cargo test or cargo nextest run will execute it from the root of the project even if you are in a subdirectory.

If there is a reason this was here I am happy to keep it. I was trying to remove it because it wasn't working under nix. Alternatively we could just depend on git inside of the nix test

@mara-schulke
Copy link
Contributor

Seems like the tests pass! Just beware that you now have a clippy error in ci

Also I think this code used to be in there to make cargo test executable in subdirectories / crates

@DavidWHelsing DavidWHelsing force-pushed the dmw/simplify-project-root-detection-in-tuto-test branch from b540ed1 to c12e752 Compare May 10, 2024 09:37
@DavidWHelsing
Copy link
Contributor Author

Also I think this code used to be in there to make cargo test executable in subdirectories / crates

I test this and it worked for me. Both using cargo test and cargo nextest run running from a subdirectory. I think they both set cwd to be the project root when executing.

But happy to not merge this if there was a reason why this was added!

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.

None yet

3 participants