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

Fix expression parsing to allow all literal method names #357

Closed
mperham opened this issue Jun 22, 2015 · 17 comments
Closed

Fix expression parsing to allow all literal method names #357

mperham opened this issue Jun 22, 2015 · 17 comments
Labels

Comments

@mperham
Copy link

mperham commented Jun 22, 2015

I'd love to use mutant to test Sidekiq's minitest suite but it fails on a legal method. I hope this is an easy fix!

/Users/mike/.gem/ruby/2.2.1/bundler/gems/mutant-cfb976e1412f/lib/mutant/expression.rb:135:in `parse': Expression: "Sidekiq.❨╯°□°❩╯︵┻━┻" is not valid (Mutant::Expression::InvalidExpressionError)
    from /Users/mike/.gem/ruby/2.2.1/bundler/gems/mutant-cfb976e1412f/lib/mutant/subject/method.rb:31:in `expression'

https://github.com/mperham/sidekiq/blob/master/lib/sidekiq.rb#L40

@mbj
Copy link
Owner

mbj commented Jun 22, 2015

Its not a reasonable ruby method name. But you have a point. People designing languages allowing unicode identifiers should be banned to hell IMO. (its 2020 now when I edit this and do not agree with my past self on UTF-8 in identifiers anymore).

I'll look into re-using the method name lexer / parser from parser instead of my home-brew one that is too restrictive, but should be accurate for all ASCII method names.

It may turn out to be to complex / maintenance intensive, than this feature request will be closed on my OSS policy that means:

  • I only implement what I need for my projects
  • Unless its easy, or beneficial for the tools adoption (that gets me better feedback to improve the tool for my needs, more testers etc).

Unicode methods are such an edge case I might consider its not worth the effort. For now this issue stays open.

@mperham
Copy link
Author

mperham commented Jun 22, 2015

Just a thought, I'd be happy to flag the method as "ignore", a la # :nodoc:

@mbj
Copy link
Owner

mbj commented Jun 22, 2015

@mperham Right now mutants architecture only allows to ignore subjects after they got identified (via parsing against that regexp that does not allow any valid ruby method name, only the ASCII subset). Given that thought I should look for adding that unicode identifiers.

@mbj
Copy link
Owner

mbj commented Jun 22, 2015

BTW, the most easiest way to overcome this would be the remove of that method, that does not serve any real propose. I personally dislike any sort of easter egg, since those lead to unneeded entropy in the source code.

Also easter eggs allow fingerprinting of the technology stack, does not apply here AFAIK.

@davydovanton
Copy link
Contributor

but what to do with such methods?

@mbj
Copy link
Owner

mbj commented Jun 22, 2015

but what to do with such methods?

Rename to something that is more descriptive for the API user?

@mbj
Copy link
Owner

mbj commented Jun 22, 2015

but what to do with such methods?
Rename to something that is more descriptive for the API user?

I realize its a private method. Still a more descriptive name helps the "internal" API user (aka library author / contributors).

@mperham
Copy link
Author

mperham commented Jun 22, 2015

I have several Unicode methods more important than that one. It's ok, Rubinius had to make changes to support Sidekiq's unicode methods too. 😄

@mbj
Copy link
Owner

mbj commented Jun 22, 2015

Yeah, I see all your points, and my resistance in supporting them shrinks with more examples ;)

@mbj
Copy link
Owner

mbj commented Jun 22, 2015

So my summary: Its IMO a ruby misdesign to allow such method names. BUT this is a ruby mutation testing engine. So I'll support it, not top priority but I'll not resist the change anymore :P

@mperham
Copy link
Author

mperham commented Jun 22, 2015

No problem, I'd love Sidekiq to be a high-profile example of how cool mutant is, with minitest support and all.

@mbj
Copy link
Owner

mbj commented Jun 22, 2015

Writing a project specific minitest integration is much easier than the "generic" one. Since the project specific minitest integration could fit the "minitest style" the project uses, and does not need to be universal. Since mutant is for good reasons an engine that selects tests by convention, and not line tracing mutant needs all that "meta information" that I get for "free" in rspec.

In fact I did project specific minitest integrations already. Maybe we should focus here, and than observe a general pattern or a building-block that minitest users can use to assemble from. Instead of shooting for the "universal" one that seems to be tricky, given the fact it takes so long.

Have to admit my minitest support fell under my OSS policy multiple times already, so I did not invested lots of time to create it. And preferred the project specific integration (faster, less client time consumed) for that commercial code where minitest integration happened sucessfully.

@mperham I'm happy to support sidekiq, also I got some sponsoring for finally shipping tracing to production (that removes the need for convention based selection) so it should be possible to get you covered.

@mperham
Copy link
Author

mperham commented Jun 22, 2015

I like that you are so clear on your OSS policy and what you will and won't do. I know OSS is a time sink so I'm glad you found sponsors to pay for work on mutant!

@mbj
Copy link
Owner

mbj commented Jun 22, 2015

I know OSS is a time sink

Yeah, If I support everyones wishes, I'll not do the important stuff. Since the feature requests are done by people not so deep into the topic. The tool would never reach the maturity to actually attract sponsoring. I'm not always right but had some success with that policy already.

@mbj
Copy link
Owner

mbj commented Jul 3, 2015

As a side effect of an internal refactoring mutant should be able to work on projects with non ASCII method names.

Work in sense of: Mutations on that methods can be killed as part of a more coarse grained selector expression that selects subjects.

So running mutant on Sidekiq* expression will be fine, but targeting a subset via the non ASCII method name is not fixed via that branch. Sidekiq#❤ as subject selection expression will not work, but that subject will be included via Sidekiq*.

I still plan to support any allowed literal method name fully, this comment is a status update for the process.

@mbj mbj changed the title Does not handle legal Ruby method names Does not handle legal literal Ruby method names in expressions Jul 13, 2015
@mbj mbj added the enhancement label Aug 8, 2015
@mbj mbj changed the title Does not handle legal literal Ruby method names in expressions Fix expression parsing to allow all literal method names Oct 23, 2015
@mbj mbj added bug and removed enhancement labels Oct 23, 2015
@mbj
Copy link
Owner

mbj commented Mar 16, 2016

Related parser issue: whitequark/parser#213

mbj added a commit that referenced this issue Dec 12, 2020
mbj added a commit that referenced this issue Dec 12, 2020
@mbj
Copy link
Owner

mbj commented Dec 12, 2020

@davydovanton, @mperham it only took 5 years till I finally got around to add support for these method names in mutant. Meanwhile mutant is a commercial tool - and I have a customer with a large non US-ASCII code base.

I tried to find the odd method names in sidekiq, but could not see them? So I tested against for the moment. If you could point me to any opensource codebase I can test mutants support for non US-ASCII method names?

@mbj mbj closed this as completed in 9ae3dc6 Dec 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants