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

Clojure native #138

Closed
wants to merge 13 commits into from
Closed

Clojure native #138

wants to merge 13 commits into from

Conversation

hiredman
Copy link
Contributor

rewrite of the clojure backend in clojure

  • includes a lein plugin
  • snippet support is not included
    • dealing with abstract classes with private and protected methods and fields in clojure is a pain, it would be nice if there was an interface to use for this
  • no changes to tests (not sure how/if the stuff in src/test runs)
  • the loading of clojure source needs some work, mostly due to the change to FileResourceLoader, but clojure always loads relative to the classpath, so at least in the lein plugin I may switch back to the ClassPathResourceLoader

@@ -78,7 +78,9 @@ public void runStep(Locale locale) throws Throwable {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class is no longer used - am I correct? Delete it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind my last comment.

@aslakhellesoy
Copy link
Contributor

RE: snippets: I'll refactor it to make it easier to use. (Replace inheritance with delegation)

RE: tests: You haven't tried to run the tests? How do you know if this works then?

RE: Changes to FileResourceLoader. What changes are you referring to? Feel free to change back to the ClasspathResourceLoader if that makes more sense.

aslakhellesoy added a commit that referenced this pull request Jan 14, 2012
@hiredman
Copy link
Contributor Author

I am interested in using cucumber for integration tests for a clojure application. in the process of writing this I also wrote my first feature and scenarios for doing the integration testing. That's how I know the code "works".

I would like to get the existing tests working, there are some files in clojure/src/test but when I run 'mvn test' nothing happens, this may be the result of me adding clojure-maven-plugin to the pom. I'm not a big maven guy, we use leiningen at work.

As I was working on this at someone point when I merged in master things broke and I had to fiddle with the paths passed in to cli.Main (this was before I stopped using cli.Main in the lein plugin) and I thought that somehow the Backend switched from getting passed a ClasspathResourceLoader to a FileResourceLoader, perhaps this is incorrect.

@aslakhellesoy
Copy link
Contributor

Did you have a chance to look at the Snippet API after I refactored it? Now you just have an interface to implement - much better separation IMO.

I haven't tried your code yet (it no longer applies cleanly with master). Can you try to run the regular ant build and make sure it works?

Currently a Backend is instantiated with a ClasspathResourceLoader when kicked off from JUnit, and a FileResourceLoader when kicked off from the CLI. This seems like a sensible default, especially since Java people will prefer the JUnit runner, while e.g. jruby/jython/rhino etc people will prefer the CLI.

Clojure seems like an exception to the rule (jruby/jython/rhino don't use the CLASSPATH), so maybe you should ignore the resourceLoader passed to the ctor and always use a ClasspathResourceLoader.

@rplevy-draker
Copy link
Contributor

I ran "mvn clean install" on the clojure-native branch and it fails when running the test cucumber.resources.ResourcesTest. That file doesn't seems to contain any actual tests however.

This doesn't occur on the master branch of cucumber-jvm, main repo (but as a side note, a Groovy test fails).

@aslakhellesoy
Copy link
Contributor

Is anyone interested in bringing this uptodate with the current master HEAD? If not I'll close this issue. /cc @hiredman @rplevy-draker @nilswloka

@nilswloka
Copy link
Member

I'm not sure whether I have sufficient Clojure skills, but I can give it a try. I'm still very much interested in a native version.

@nilswloka
Copy link
Member

Sent a pull request: #265

@lock
Copy link

lock bot commented Oct 25, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants