-
Notifications
You must be signed in to change notification settings - Fork 191
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
Use walkDirRec for nimble test, output to build/tests #850
base: master
Are you sure you want to change the base?
Use walkDirRec for nimble test, output to build/tests #850
Conversation
CI is failing since this functionality is explicitly disabled - see Best to wait for @dom96 to confirm why this is not supported. It might very well be a breaking change but seems like a reasonable ask as well. |
Thanks, don't mean to be a bother. I was just starting a project in Nim and this particular thing just bothered me is all. |
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.
Seems fine to me in principle, but please revert the code style changes, most are wrong and they make review harder.
optsCopy.getCompilationFlags().add("--out=" & ("build/" / | ||
file.changeFileExt(ExeExt))) |
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.
hm, don't we have a setting for this nowadays? @genotrance?
If not, maybe it would be better to put this in tests/build/
or tests/bin/
?
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.
Not sure what setting?
Related: #787
I think we should instead rely on nim r
instead of worrying about yet another directory. That's only in devel though so we should wait a bit until it is in stable.
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.
My problem with relying on nim r
is that it can leave executable files littered in folders, since in the original code you have to do a 'existedBefore' and 'existedAfter' check. If executable files, due to some crash or whatever reason, were not cleaned up properly, you now have these files scattered all over the test folder's subdirectories that will -not- get removed next 'nimble test' run (because existedBefore will now be true, so they won't get trashed).
Traditionally, a build folder is very clear that it's just build output and can be safely deleted.
I guess tests/build
could work.
I just think it's important to have this discussion because it would make nimble test
plug and play.
My workflow is to mirror my 'tests' folder based on my src folder, more or less.
So if I had a src/foo/foo_database.nim
, it would have a corresponding tests/foo/test_foo_database.nim
What's the status of this? Can we get those formatting changes removed? |
ping |
This is not meant to be added into master directly, there's probably a lot wrong with this code style-wise but it works for me.
Sorry about the whitespace, I do use nimpretty so maybe the original file wasn't formatted?
It serves as an illustration to this issue: #849
This is how I expected
nimble test
to actually work, not whatever it is doing now.