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

Removing 'new' class method from interfaces #3275 #3752

Merged
merged 2 commits into from Apr 27, 2016
Merged

Conversation

@Lan5432
Copy link
Contributor

Lan5432 commented Mar 22, 2016

resolves #3275

@Lan5432 Lan5432 changed the title Fixing for issue #3275 Removing 'new' class method from interfaces #3275 Mar 22, 2016
Added a new spec: 'interface_new.rb'

See issue #3275
@kares
Copy link
Member

kares commented Mar 22, 2016

looking good thanks, we'll wait for the travis build: https://travis-ci.org/jruby/jruby/builds/117694824
... and if @headius and @enebo are OK with this, we could merge for 9.1

@kares
Copy link
Member

kares commented Mar 22, 2016

seems that you might have not run the spec:ji suite locally - there are failures, so this will need more thought and work: https://travis-ci.org/jruby/jruby/jobs/117694834 (haven't looked at the failures myself)

@Lan5432
Copy link
Contributor Author

Lan5432 commented Mar 22, 2016

I answered your email about that, even after properly installing spec I had problems to use it. It might be the errors you mentioned.

@kares
Copy link
Member

kares commented Mar 23, 2016

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. rake spec:ji runs on travis thus if you follow how .travis.yml boots up you should end up the same.

regarding the actual failures we believe those are just poorly written specs that need a rewrite but first make sure you reproduce the failures.

@Lan5432
Copy link
Contributor Author

Lan5432 commented Mar 23, 2016

So I try to run raje spec:ji and see if the the failures are the same to ascertain where the problem is?

@enebo
Copy link
Member

enebo commented Mar 23, 2016

@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.

@Lan5432
Copy link
Contributor Author

Lan5432 commented Mar 24, 2016

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?

@enebo
Copy link
Member

enebo commented Mar 24, 2016

@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.

@Lan5432
Copy link
Contributor Author

Lan5432 commented Mar 25, 2016

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.

describe "Needed implementation methods for interfaces" do
  it "should have __id__ method" do
    expect(BeanLikeInterface.new.methods).to have_strings_or_symbols("__id__")
  end
  it "should have __send__ method" do
    expect(BeanLikeInterface.new.methods).to have_strings_or_symbols("__send__")
  end
  it "should have == method" do
    expect(BeanLikeInterface.new.methods).to have_strings_or_symbols("==")
  end
.......

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.

@kares
Copy link
Member

kares commented Mar 25, 2016

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.

@Lan5432
Copy link
Contributor Author

Lan5432 commented Mar 26, 2016

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.

@kares
Copy link
Member

kares commented Mar 29, 2016

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 ... Runnable.impl) like before.

@Lan5432
Copy link
Contributor Author

Lan5432 commented Mar 29, 2016

Oh, right, so we should test it on the result of invoking impl so they are tested upon interface instances

@kares
Copy link
Member

kares commented Mar 30, 2016

@Lan5432 looks good, thanks! @enebo any objections for merging this along with 9.1's JI updates ?

@kares kares added this to the JRuby 9.1.0.0 milestone Apr 14, 2016
@headius
Copy link
Member

headius commented Apr 27, 2016

Looks ok to me. 👍

@kares kares merged commit 6f98597 into jruby:master Apr 27, 2016
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.