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

Using eval for expectations prevents testing other execution modes #1152

Closed
headius opened this issue Jun 5, 2024 · 2 comments · Fixed by #1155
Closed

Using eval for expectations prevents testing other execution modes #1152

headius opened this issue Jun 5, 2024 · 2 comments · Fixed by #1155

Comments

@headius
Copy link
Contributor

headius commented Jun 5, 2024

In e922a17 all of the pattern specs were switched to using eval, presumably because TruffleRuby could not parse/compile all of the pattern forms being tested. Unfortunately this means that these examples are only ever being tested in eval.

On JRuby, this means they will never be run through our bytecode JIT, which does not compile bare eval contents. This led to jruby/jruby#8283 being missed until now. I believe this would also affect any testing of CRuby's YJIT, since I believe they also only JIT bodies of code, not eval snippits.

I understand the difficulty of running specs when syntax is not yet supported, but this is not a good solution. At the very least the contents of the eval should include a method or block wrapper, so that implementations that JIT only on those edges can still force JIT to happen for testing.

headius added a commit to headius/spec that referenced this issue Jun 5, 2024
This replaces most evals of patterns with evaluation into and
invoking of a temporary method. The remaining cases depend on
surrounding state that would need to be moved into the eval string
to work properly.

Partially addresses ruby#1152
headius added a commit to headius/spec that referenced this issue Jun 5, 2024
This replaces most evals of patterns with evaluation into and
invoking of a temporary method. The remaining cases depend on
surrounding state that would need to be moved into the eval string
to work properly.

Partially addresses ruby#1152
@headius
Copy link
Contributor Author

headius commented Jun 5, 2024

An attempt to improve this is in #1153, but I could not evaluate all cases into methods because some depend on captured variables. I would be happier if we could eliminate the evals altogether.

@eregon
Copy link
Member

eregon commented Jun 5, 2024

presumably because TruffleRuby could not parse/compile all of the pattern forms being tested.

The original reason is because 2.5/2.6 cannot parse it.
But then indeed we needed to keep as long as TruffleRuby could not parse pattern matching.
Since 3.0 is minimum now and TruffleRuby can parse (and supports) pattern matching I think there is no need to keep in eval.
If a Ruby wants to run ruby/spec but doesn't support parsing pattern matching they could exclude that file.
Could you remove the evals from that file?

headius added a commit to headius/spec that referenced this issue Jun 6, 2024
Some evals remain for SyntaxError tests that would not parse in
the main script.

Most of this was done using automated tools so a few irrelevant
formatting changes may have crept in.

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

Successfully merging a pull request may close this issue.

2 participants