-
-
Notifications
You must be signed in to change notification settings - Fork 161
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
Add test generator #728
Add test generator #728
Conversation
e9faf34
to
2edba7c
Compare
9cd4520
to
328fc65
Compare
I'll review the actual templates from the PR before we'll merge, as they should be added in separate PRs |
490df82
to
66cb526
Compare
|
||
(deftest one | ||
(is (= "I" (roman-numerals/numerals 1)))) | ||
(deftest roman-numerals_test_1 |
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.
If the tested function has dashes in it (like roman-numerals
), do we want to keep that in the function name or would you rather replace it with underscores?
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.
I don't have any strong preference. We can keep them to indicate more clearly the tested function, and use underscores for the _test_1 part. Or, we can use dashes everywhere so that we follow the language conventions. Or, we can use all underscores. Do you have any preference or suggestion?
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.
I don't. I'm happy to go with what you prefer
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.
Let's go with your suggestion then. Dashes for the function name, underscores for the _test_1 part.
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.
I'll also fix the names in my open PRs. Should be trivial to update them all. Will notify when they are done.
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.
@ErikSchierboom All my open PRs are finished, but we both have PRs for pangram and difference-of-squares. Take a look at the pangram PR to see how the tests should be written in that case. We should be explicitly checking for true or false return values. The reason for it is because without explicit checks, someone can pass the tests without actually returning true or false. And given the function ends in ?, we are expecting only true and false to be returned.
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.
I'll go through your PRs later and approve them. Once they're merged I'll update my PRs to just add the template.
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.
And I agree with true?
and false?
I'll check to see if there is a nice handlebars library, as I think we'll be needing conditionals |
I'm switching to hbs for handlebars compatible templates which are nicer to work with and extensible. I'll do some work on more involved exercises |
If you also post the isogram tests in a new PR, i'll close #705 Edit: Actually, i can't close it as i've updated more than the tests. So for isogram, only the template needs to go in a new PR. |
(defn- transform [slug test-cases] | ||
(let [transform-file (paths/generator-clojure-file slug)] | ||
(if (.exists transform-file) | ||
(let [generator-ns (symbol (str slug "-generator"))] | ||
(load-file (str transform-file)) | ||
(if-let [transform-fn (ns-resolve generator-ns (symbol "transform"))] | ||
(transform-fn test-cases) | ||
test-cases)) | ||
test-cases))) |
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.
This now allows for a generator.clj
file next to the generator.template
file that can be used to customize the data before it is passed to the template.
I've happy with what I have now, and it should be enough for a lot of exerciss. |
Open question before we'll merge PRs regarding the template: do we want |
I think I'll go with |
Co-authored-by: Anastasios Chatzialexiou <16361161+tasxatzial@users.noreply.github.com>
@tasxatzial should be good now! |
@tasxatzial I'll wait for this PR to be merged and then I'll double check all the other PRs |
The sum of multiples template has been added here accidentally. |
Ok, i'll do it as well. |
Fixed! |
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.
Ok, let's proceed then. I believe this is good to go. Feel free to merge.
@tasxatzial This is a very basic test generator prototype. The way it works is that you create a
generator.template
file in the exercise's.meta
directory and then runbin/generate-tests <slug>
. This then:problem-specifications
repo locally (or pulls the latest when it is already there)canonical-data.json
file to find the test casestests.toml
file to exclude test cases that are set toinclude = false
in thetests.toml
file (this happens automatically for reimplemented test cases)generator.template
file using the fetched test casesThe generator itself is located in the
generators
directory and uses adeps.edn
file to manage its dependency.The generator doesn't yet support any pre-processing of canonical data, but I imagined that to be done via an optional
generator.clj
file in the.meta
directory.