Skip to content

Default to CRuby for https://try.ruby-lang.org/ #148

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

Merged
merged 9 commits into from
Jul 28, 2023

Conversation

eregon
Copy link
Member

@eregon eregon commented Mar 7, 2023

Unfortunately some tests fail, I'm not sure why.
For example, running bundle exec rake spec this one fails:

rspec './spec/tutorial_spec.rb[1:1:5]' # Tutorial in en has a working step 4 (Putting it differently)

@hmdne
Copy link
Collaborator

hmdne commented Mar 7, 2023

I have attempted it and mentioned it briefly in my last refactoring PR. The tests test the lessons which are, as of now, dependent on Opal idiosyncracies, like a particular syntax of errors. If we switched the default engine for lessons, many of them would stop working.

@eregon
Copy link
Member Author

eregon commented Mar 7, 2023

The CI shows 25 failures for 117 examples, it doesn't seem too bad.
And Tutorial step 4 fails for multiple languages, likely the same fix for all.
I think we should adapt the tutorial expectations/regexps/etc so they work with CRuby WASI.

Do you have any idea how to fix that Tutorial step 4 (Putting it differently)?
I tried to look into it but unfortunately the error output of specs is unhelpful and the regexp seemed OK (seems to accept any number, e.g not depending on a floating point number result which is Opal-specific), so it's not clear why the spec would fail.
Running the website locally that example behaved fine but indeed didn't show the green "label".

@hmdne
Copy link
Collaborator

hmdne commented Mar 8, 2023

Do note, that we only test first 5 examples for non-English languages on CI so it runs faster.

I think that the reason is as follows:

https://github.com/ruby/TryRuby/blob/master/app/try_ruby.rb#L400

@output_buffer for Opal may store single lines, while for Ruby-WASI it may store multiple lines at once. A fix could be to correct this behavior.

@hmdne
Copy link
Collaborator

hmdne commented Mar 8, 2023

This was my theory and it turned out to be correct. But it's quite a bit deeper:

As of now:

puts "abc"
puts "abc"

gives for MRI the following @output_buffer: ["abc\nabc\n"]
gives for Opal the following @output_buffer: ["abc\n", "abc\n"]

This is for example the lesson "Putting it differently"

But this:

print "abc\nabc\n"

gives for both the following @output_buffer: ["abc\nabc\n"]

This is for example the lesson "Ready, Aim"

The patch I tried:

diff --git a/app/try_ruby.rb b/app/try_ruby.rb
index f38351c..9ae90a7 100644
--- a/app/try_ruby.rb
+++ b/app/try_ruby.rb
@@ -397,7 +397,7 @@ class TryRuby
     # Do not check the answer if there is no regexp matcher
     if @current_item && @current_item.answer
       # Get last line of output
-      value_to_check = @output_buffer.length > 0 && !@output_buffer.last.empty? ? @output_buffer.last.chomp : ''
+      value_to_check = @output_buffer.join.split("\n").reject(&:empty?).last
 
       # Check if output matches the defined answer regexp
       # and print status message

This gives us a way forward: we can force the value_to_check to denote only the last message then fix the lessons to rely on that.

Keep in mind, that this check is done on Opal, and Opal has another incompatibility - the regular expressions have JS semantics, so ^ means \A and $ means \z.

@eregon eregon force-pushed the default-cruby branch 2 times, most recently from 5d91b4a to b3ab990 Compare July 28, 2023 12:13
@eregon eregon marked this pull request as ready for review July 28, 2023 14:07
@hmdne
Copy link
Collaborator

hmdne commented Jul 28, 2023

One nitpick - the JSON files are generated from respective files from translations directory. So those files need to be edited first and JSON will be generated from them.

@eregon
Copy link
Member Author

eregon commented Jul 28, 2023

Ah that's why the CI fails most likely but the specs pass locally, thanks for tip!

@eregon
Copy link
Member Author

eregon commented Jul 28, 2023

Is there any reason source/try_ruby_*.json are checked-in in the repo?
It seems confusing and error-prone to have such duplication.
These files can regenerated just fine if they don't exist.

Maybe it's for convenience of deploying to https://try.ruby-lang.org/ without extra commands in between?

@eregon
Copy link
Member Author

eregon commented Jul 28, 2023

Now all files should be in sync and the CI should pass.
(but it's taking some time to show up, https://www.githubstatus.com/ )

@hmdne
Copy link
Collaborator

hmdne commented Jul 28, 2023

Is there any reason source/try_ruby_*.json are checked-in in the repo? It seems confusing and error-prone to have such duplication. These files can regenerated just fine if they don't exist.

Maybe it's for convenience of deploying to https://try.ruby-lang.org/ without extra commands in between?

Not sure. I agree it makes no sense to keep that.

Now all files should be in sync and the CI should pass. (but it's taking some time to show up, https://www.githubstatus.com/ )

I'm running the full suite of tests locally and once they pass, I will merge.

@hmdne
Copy link
Collaborator

hmdne commented Jul 28, 2023

All good. Great work!

@eregon
Copy link
Member Author

eregon commented Jul 28, 2023

Thanks for review and merging.
I'm delighted https://try.ruby-lang.org/ now uses CRuby!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it clearer Opal is used on https://try.ruby-lang.org/
2 participants