-
Notifications
You must be signed in to change notification settings - Fork 1
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
Testing setup #6
Conversation
…ectory and Makefile, add new test cases
…d testing functionality for subset of paths
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.
Looking very awesome!!! Here are a few comments throughout. I have a few Makefile suggestions, centered on its ability to do interesting pattern substitution with syntax that looks like $(VAR:%.foo=%.bar)
, which substitutes every element in the value of the variable VAR
by looking for .foo
and swapping that for .bar
.
test/*.og | ||
test/*.gfa | ||
test/*.out |
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.
Just checking: did you mean to ignore the .out
files? The usual setup with Turnt is to keep them checked into the repository. That way, if someone else clones the repository, they can also verify that the output matches the expected result. (Of course, it's impractical if the output files are gigantic—do you know how big they are for the tests we currently have?)
Makefile
Outdated
clean: | ||
rm -rf test/*.gfa | ||
test: og | ||
-turnt --save --env baseline $(foreach file,$(TEST_FILES),test/$(file).gfa) |
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.
Ah, I see! Possibly belay my comment above about ignoring the .out
files. Used like this to automatically generate a baseline, perhaps we don't want to check in the expected output files at 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.
My intuition was that it is more in keeping with the philosophy of differential testing to generate the .out
files every time we run the tests, especially since we get some of our tests from the odgi repo, and these tests could theoretically change without us knowing about it.
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.
Sounds great 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.
Yay; this all looks great!!! Thanks for bearing with the latency on this review, but this all looks good to go. Feel free to do the honors and push that green button!
This PR does a couple of things:
test
calledsubset-paths
which contains.paths
files. Each.path
file specifies a subset of paths for some.og
file. Additionally, for each{base}.paths
file there is a{base}.txt
file which specifies the.og
file which{base}.paths
corresponds to. For example, there is a file calledex1.paths
, so there is a correspond fileex1.txt
which simply contains the lineARGS: ../basic/ex1.og
, specifying thatex1.paths
lists a subset of the paths in../basic/ex1.og
. (I cannot put comments in the.paths
files because of how odgi parses them. :( )This approach to testing, while requiring these
.txt
files, allows us to keep all of the.paths
files in the same directory even though the.gfa
files may be in different directories, and it allows us to generate multiple.paths
files for the same.og
files without duplicating the.og
files and placing them in separate directories.The Makefile is a bit... messy. The target
make og
is especially difficult for the human eye to parse. If you have time and feel like messing with Makefiles, I would especially appreciate if you could take a look and let me know how this could be done better!