Add specs for top-level return #530

andrykonchin · 2017-10-21T16:53:41Z

Top-level return is now allowed. Feature #4840

Related issue #473

Following test cases aren't based on feature discussion or ruby tests and just describe current behavior so I am not sure they should be added:

"within if"
"within while loop"
"within a block"
"within a class"

Moreover according to discussion using of return keyword in class and block is incorrect but it works for Ruby 2.4.2

andrykonchin · 2017-10-23T16:52:49Z

The build for Ruby 2.4 on AppVeyor is failed because Ruby 2.4.1p111 is used.

Top level return is a new feature and is still raw. In 2.4.2 some bugs were fixed and looks like they will polish it in 2.5 and later.

So I don't know the right solution. I see two ways

last stable version (2.4.2) is used on AppVeyor or
I have to add version guards for 2.4.1 to test its specific bugs and behavior.

eregon · 2017-10-23T17:05:44Z

In general, it's better to update to latest releases if possible.
You could make the guard tighter with ruby_version_is "2.4.2", after all the feature is incomplete before that.

This is odd, AppVeyor should use 2.4.2 according to https://www.appveyor.com/docs/build-environment/#ruby.
@MSP-Greg Do you know something about this? How can we use 2.4.2? Or is this because we use the 64-bit build of 2.4?

MSP-Greg · 2017-10-23T18:12:13Z

@eregon

Appveyor just updated from 2.4.1 to 2.4.2 yesterday 22-Oct-17.

For current Appveyor info, I've got a run of ruby related info at appveyor-ruby. It's run twice a day, shows info on all the minor versions. It also shows all the MSYS2 / MinGW packages, which are used to build both ruby and extension gems.

I'm not sure about 2.2.8 or 2.3.5, as they haven't been released yet. FYI, 2.3 and earlier were/are built with a proprietary system based on RubyInstaller (MSYS / MingW). 2.4 forward (and my trunk build) are built with MSYS2 / MinGW, and utilize the RubyInstaller2 runtime.

Probably more info than you were looking for... Thanks.

MSP-Greg · 2017-10-23T18:15:53Z

@eregon

I looked recently, and noticed that Travis ruby-head and Appeyor trunk were both failing on 'Mock.verify_count' issues and one array spec. I may have a go at it, but probably won't be for a few days...

andrykonchin · 2017-10-24T09:34:10Z

@eregon looks like we need just to re-run build on AppVeyor. I still don't have permissions. Could somebody do it?

eregon · 2017-10-24T10:37:46Z

I restarted the build. For some reason, AppVeyor is on my personal GitHub account, so I guess I'm the only one with permissions. I should try to fix that.

eregon · 2017-10-24T10:44:38Z

@MSP-Greg Ah that explains why, thanks for the info!
https://ci.appveyor.com/project/MSP-Greg/appveyor-ruby is very nice.

Re ruby-head/trunk, it is expected to fail, as basically we are testing latest of ruby/ruby with latest of ruby/spec and they are only synchronized ~once a month. Maybe we should just remove those from CI as they are only useful when I do synchronization (and even then, the Travis ruby-head build is often old).
For ruby-head, what matters is that the copy ruby/spec under <ruby_repo>/spec/ruby passes, not latest ruby/spec.

mjago · 2017-10-24T11:32:00Z

For ruby-head, what matters is that the copy ruby/spec under <ruby_repo>/spec/ruby passes, not latest ruby/spec.

Should ruby-head specs go to ruby/spec/ruby ?

I ask since I'm currently writing specs that differ between 2.4.2 and ruby-head and am writing them together to save effort.

Perhaps an event can be setup to notify of changes made to ruby/spec/ruby so they can be tested (travis head as currently) and pulled into here more easily?

eregon · 2017-10-24T11:57:13Z

Either is fine (ruby/ruby or ruby/spec), it's OK to add them here, but of course test locally with latest ruby-trunk then.

Perhaps an event can be setup to notify of changes made to ruby/spec/ruby so they can be tested (travis head as currently) and pulled into here more easily?

Automatically importing specs changes from ruby/ruby would be nice indeed. I guess it's not so trivial, particularly when there are conflicts (rare overall but still occasionally happens).

eregon · 2017-10-24T11:59:51Z

There is a script to help synchronizing specs across repos, but it's still partially manual.
@mjago Could you open an issue if you think this is worth trying?

mjago · 2017-10-24T12:21:03Z

@eregon will do

MSP-Greg · 2017-10-24T12:42:50Z

@eregon

Maybe we should just remove those from CI as they are only useful when I do synchronization (and even then, the Travis ruby-head build is often old).

No reason to have surprises once things are moved to ruby/spec/ruby.

I suppose test frameworks are messy in that two different types of work are done, one to make the tests more complete for existing functionality, and second, to add more testing for new trunk functionality.

As long as everyone working in here is aware of trunk test breakage while WIP, shouldn't be an issue...

Thanks.

eregon

These are nicely readable specs!

Could you try removing those ruby_exe? They make specs slower and don't seem needed here.

eregon · 2017-10-26T18:43:02Z

language/return_spec.rb

+
+          ruby_exe(<<-END_OF_CODE).should == "before return\n"
+            load "#{temp.path}"
+          END_OF_CODE


The file needs to be removed after the end of the spec.
It's also better to use the tmp("loaded_file.rb") and rm_r helpers as they avoid creating any file outside the rubyspec_temp directory.

eregon · 2017-10-26T18:48:35Z

language/return_spec.rb

+          puts "after return"
+        END_OF_CODE
+
+        $?.exitstatus.should == 0


ruby_exe is powerful but has the cost of spawning a new Ruby process every time.
This is particularly bad on some implementations where it can take anywhere from 10ms to a couple seconds.

I think those specs do not need ruby_exe, instead they could use a tmp("top_return") file (in a before :each block), File.write() the code in it and just call Kernel#load.
Then the puts should be replaced with ScratchPad << "before return" and ScratchPad.recorded.should == ...

andrykonchin · 2017-10-26T21:48:09Z

language/return_spec.rb

+      after do
+        rm_r @filename
+      end
+
      it "stops file execution" do
        ruby_exe(<<-END_OF_CODE).should == "before return\n"


@eregon
I left ruby_exe here intentionally to test terminating the ruby process

Good idea, it's worth testing this also works for the main script.

andrykonchin · 2017-10-26T21:49:57Z

language/return_spec.rb


-          $?.exitstatus.should == 0
+        it "does not fire ensure block before returning while loads file" do


@eregon it looks like a Ruby bug and I am not sure this case should be present. Previous not changed case illustrates the feature

That looks like a bug indeed. I reported it at https://bugs.ruby-lang.org/issues/14061.
Please annotate it with ruby_bug "#14061", "2.4" and specify the opposite behavior inside, i.e., the expected non-buggy behavior.

andrykonchin · 2017-10-26T21:50:36Z

@eregon done

eregon · 2017-10-27T09:54:51Z

language/return_spec.rb

+
+          ->() {
+            load @filename
+          }.should_not raise_error


Minor, but should_not raise_error is not so useful, leaving outside the lambda would be clear it's expected to not raise an error. I'll remove it.
I actually want to remove all should_not raise_error in ruby/spec, because positive expectations are much more precise, but there are quite a few of them.

@eregon should_not raise_error expresses the test case intention here. If feature is broken and exception propagates with such raise_error expectation test will fail. Without raise_error expectation test will considered as just broken/outdated.

It's a minor issue and if you consider should_not raise_error expectation as excessive - I will remove it.

Yes, it makes a difference between an error and failure in mspec.
I personally see no value to that difference. I see it as either the spec passes or it does not.
If the spec fails because of a method returning a different value or because it raises an exception it's the same to me, it doesn't pass and needs fixing :)
If the spec raises an exception, the interpreter is behaving wrong whether it's an "error" or "failure" (or the spec is broken of course).

eregon · 2017-10-27T10:11:30Z

language/return_spec.rb

+      end
+
+      describe "return with argument" do
+        it "does not affect exit status" do


This seems also a bug, filed as https://bugs.ruby-lang.org/issues/14062.
It seems OK to keep the spec like this for now though, could you just add a comment with the bug URL above?

andrykonchin · 2017-10-27T17:54:46Z

@eregon done

eregon

Thank you for the new specs and congrats for finding 2 bugs in one PR!

Add specs for top-level return (tested with Ruby 2.4.2)

b39b5ba

eregon mentioned this pull request Oct 24, 2017

Write specs for new Ruby 2.4 features and changes #473

Closed

70 tasks

mjago closed this Oct 24, 2017

mjago reopened this Oct 24, 2017

andrykonchin changed the title ~~Add specs for top-level return (tested with Ruby 2.4.2)~~ Add specs for top-level return Oct 25, 2017

eregon reviewed Oct 26, 2017

View reviewed changes

Code review. Replace excessive ruby_exe calls with temp file loading

d75036d

andrykonchin commented Oct 26, 2017

View reviewed changes

eregon reviewed Oct 27, 2017

View reviewed changes

Code review. Mark some test cases as bugs

c580407

eregon approved these changes Oct 28, 2017

View reviewed changes

eregon merged commit d9c1c83 into ruby:master Oct 28, 2017

andrykonchin deleted the add-specs-for-top-level-return branch October 28, 2017 14:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add specs for top-level return #530

Add specs for top-level return #530

andrykonchin commented Oct 21, 2017 •

edited

Loading

andrykonchin commented Oct 23, 2017

eregon commented Oct 23, 2017

MSP-Greg commented Oct 23, 2017

MSP-Greg commented Oct 23, 2017

andrykonchin commented Oct 24, 2017

eregon commented Oct 24, 2017

eregon commented Oct 24, 2017

mjago commented Oct 24, 2017

eregon commented Oct 24, 2017 •

edited

Loading

eregon commented Oct 24, 2017

mjago commented Oct 24, 2017

MSP-Greg commented Oct 24, 2017

eregon left a comment

eregon Oct 26, 2017

eregon Oct 26, 2017

andrykonchin Oct 26, 2017

eregon Oct 27, 2017

andrykonchin Oct 26, 2017 •

edited

Loading

eregon Oct 27, 2017

andrykonchin commented Oct 26, 2017

eregon Oct 27, 2017 •

edited

Loading

andrykonchin Oct 27, 2017 •

edited

Loading

eregon Oct 27, 2017

eregon Oct 27, 2017

andrykonchin commented Oct 27, 2017

eregon left a comment


		$?.exitstatus.should == 0
		it "does not fire ensure block before returning while loads file" do

Add specs for top-level return #530

Add specs for top-level return #530

Conversation

andrykonchin commented Oct 21, 2017 • edited Loading

andrykonchin commented Oct 23, 2017

eregon commented Oct 23, 2017

MSP-Greg commented Oct 23, 2017

MSP-Greg commented Oct 23, 2017

andrykonchin commented Oct 24, 2017

eregon commented Oct 24, 2017

eregon commented Oct 24, 2017

mjago commented Oct 24, 2017

eregon commented Oct 24, 2017 • edited Loading

eregon commented Oct 24, 2017

mjago commented Oct 24, 2017

MSP-Greg commented Oct 24, 2017

eregon left a comment

Choose a reason for hiding this comment

eregon Oct 26, 2017

Choose a reason for hiding this comment

eregon Oct 26, 2017

Choose a reason for hiding this comment

andrykonchin Oct 26, 2017

Choose a reason for hiding this comment

eregon Oct 27, 2017

Choose a reason for hiding this comment

andrykonchin Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

eregon Oct 27, 2017

Choose a reason for hiding this comment

andrykonchin commented Oct 26, 2017

eregon Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

andrykonchin Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

eregon Oct 27, 2017

Choose a reason for hiding this comment

eregon Oct 27, 2017

Choose a reason for hiding this comment

andrykonchin commented Oct 27, 2017

eregon left a comment

Choose a reason for hiding this comment

andrykonchin commented Oct 21, 2017 •

edited

Loading

eregon commented Oct 24, 2017 •

edited

Loading

andrykonchin Oct 26, 2017 •

edited

Loading

eregon Oct 27, 2017 •

edited

Loading

andrykonchin Oct 27, 2017 •

edited

Loading