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

Allow explicit test root #5980

Merged
merged 3 commits into from
Jan 2, 2025
Merged

Allow explicit test root #5980

merged 3 commits into from
Jan 2, 2025

Conversation

bspeice
Copy link
Contributor

@bspeice bspeice commented Jan 1, 2025

Intended to address #5979.

From what I can tell, this should be a non-breaking change. The existing tests still run correctly, and I've verified on my computer that I can use an arbitrary test directory with the -test-dir option.

While attempting to implement this, I did also notice an off-by-one issue with the binary name on Linux. While _calcExectuablePath properly null-terminates the program path, it overwrites the final non-null character of the name. This ends up breaking functions like Path::getCanonical later on, because slang-tes does not exist. If that change should be in a separate PR, please let me know.

@bspeice bspeice requested a review from a team as a code owner January 1, 2025 20:26
@CLAassistant
Copy link

CLAassistant commented Jan 1, 2025

CLA assistant check
All committers have signed the CLA.

csyonghe
csyonghe previously approved these changes Jan 1, 2025
@csyonghe csyonghe added the pr: non-breaking PRs without breaking changes label Jan 1, 2025
@csyonghe
Copy link
Collaborator

csyonghe commented Jan 1, 2025

This will make slang-test easier to use during development. Thank you for contributing the patch!

@csyonghe csyonghe enabled auto-merge (squash) January 1, 2025 23:30
@csyonghe csyonghe linked an issue Jan 2, 2025 that may be closed by this pull request
@csyonghe
Copy link
Collaborator

csyonghe commented Jan 2, 2025

The change seems to break existing tests.

@bspeice
Copy link
Contributor Author

bspeice commented Jan 2, 2025

...and specifically on Windows for some reason. Will take a look at what's going on, thanks for the heads up.

auto-merge was automatically disabled January 2, 2025 01:07

Head branch was pushed to by a user without write access

@bspeice
Copy link
Contributor Author

bspeice commented Jan 2, 2025

Ah, tests are breaking because of an unintentional change in the path separator. I was using tests as the directory name, which led Windows to use filenames like test\.... Using a directory name of tests/ fixes things up for me locally, waiting on actions to complete to make sure it's fixed for good.

@csyonghe csyonghe enabled auto-merge (squash) January 2, 2025 02:18
@csyonghe csyonghe merged commit e3b71cf into shader-slang:master Jan 2, 2025
19 of 20 checks passed
@bspeice bspeice deleted the test-dir branch January 2, 2025 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: non-breaking PRs without breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support arbitrary test directories
3 participants