-
Notifications
You must be signed in to change notification settings - Fork 412
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: exhibit a bug when promoting directories #11213
base: main
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,43 @@ | |||
Tests for promoting directory targets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally we try to avoid cram test folders with multiple files. Can you rename this file into deep-subdir.t
and inline the dune
and dune-project
?
Otherwise the test looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! And sorry for the delay: I missed the notification...
I now see that @ElectreAAS has incorporated this PR into #11226, with your suggestion and along with a fix (thanks a lot!). Should I close this one then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you want, I incorporated your test because I wanted to be sure I wasn't making any existing bug worse, but that doesn't mean you have to close this PR
If you can apply Marek's suggestion we can merge this and I'll modify #11226 accordingly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. (And signed the commits.)
Signed-off-by: Paul-Elliot <[email protected]>
Signed-off-by: Paul-Elliot <[email protected]>
f8bb15b
to
87c7fb1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests needs to be made reproducible but otherwise looks good :-)
P.S.: Yes, I agree with @ElectreAAS. It's generally nice to merge repro cases first to show that PR that fixes it solves the issue.
|
||
$ dune build deep_copied --verbose | ||
Shared cache: enabled-except-user-rules | ||
Shared cache location: /home/panglesd/.cache/dune/db |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would the non --verbose
run be enough? Otherwise you need to figure out paths and things that might change.
This shows a reproduction for issue #11214!