-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
Removing 'new' class method from interfaces #3275 #3752
Conversation
Added a new spec: 'interface_new.rb' See issue #3275
looking good thanks, we'll wait for the travis build: https://travis-ci.org/jruby/jruby/builds/117694824 |
seems that you might have not run the |
I answered your email about that, even after properly installing spec I had problems to use it. It might be the errors you mentioned. |
e-mail? sure did not ask about that in a mail as much as I recall :) share your issues here and what you tried (you should see the same few failures as on the linked travis job above). you won't be able to do much JI work without being able to run these. regarding the actual failures we believe those are just poorly written specs that need a rewrite but first make sure you reproduce the failures. |
So I try to run |
@Lan5432 correct. ./bin/jruby -S rake spec:ji should show a bunch of errors with AbstractBeanInterface.new or something like that. but it should be as simple as what I just provided. If it is not then something is wrong with your dev env. |
OK, I ran the specs with my build (which I think I should update with the commits made to the upstream repository) but the results are similar if not the same (didn't check each line) https://gist.github.com/Lan5432/761c1e7ee4886d90f38d So what does this mean? My solution does not work for some tests? |
@Lan5432 Yeah without your commit(s) those 11 failures do not happen. I think both @kares and I think these are bad tests but it would be good for you to examine them and either remove them and/or try and figure out what they may be trying to test. You can also talk to us on irc as part of that. So figure out the actual specs and the file they occur in and then see what you can figure out about motivation behind why they exist. |
Ok, so the problem is that my commit makes it impossible to instantiate interfaces. Which is what jruby/spec/java_integration/methods/naming_spec.rb is doing a lot. If you check the result of the tests, it complains mainly about this file and the invocations upon BeanLikeInterface.
Took the file out and ran the tests, no failures ( https://gist.github.com/Lan5432/b93b4af83083f3c2cf33 ) So we need to change that for another way of reading method list, because it's the problem with the specs. |
yes but the intent of those specs is smt different - they can be tested without instantiating an interface. do you think you can grasp and refactor those? we do not want do remove them. |
So I modified the spec giving problems and I removed the keyword 'new' hoping interfaces can yield the method list with just using ".methods" I ran the tests and I didn't see any failure. |
the spec describes "Java instance method names" which need to "have implementation methods for interfaces" ... the tricky part is that interfaces have some special method routing compared to "normal" modules. thus while your change might be finy it would be better if we also tested routing on actual "interface instances" (which we already talked before how to generate ... |
Oh, right, so we should test it on the result of invoking impl so they are tested upon interface instances |
Looks ok to me. 👍 |
resolves #3275