smoosh icon indicating copy to clipboard operation
smoosh copied to clipboard

Consider variable assignments during command search (rehash of #20)

Open mgree opened this issue 5 years ago • 4 comments

The original fix in #20 looked good and tested well... but is breaking something in Modernish. Needs investigation.

mgree avatar Jun 03 '20 17:06 mgree

I checked it locally, it does not break anything. Standing on 6156f8f1cbc8e381a44a6c3ee6a0a82a60f42c70 modernish runs 116 tests and then is killed because of the issue #20 fixed. Standing on 84cb012ffcb88596313bdc2384e953b7315360b1 it runs 234 tests and then gets stuck in an endless loop in shellquote:001. That loop is caused by #21 and it only fixed by that ugly hack mentioned in that issue. You can run the offending test with this:

smoosh modernish/bin/modernish --test -t mapr:003

tucak avatar Jun 03 '20 18:06 tucak

Hmm... are you on macOS? I wonder if I'm hitting something else weird, or if there's some other artifact of my local setup. Lem has been complaining about my numerics... I presume due to some local changes of mine.

I'm happy to try folding in #21, but I'm starting to have trouble keeping track of all the moving parts!

mgree avatar Jun 03 '20 18:06 mgree

No, I'm on GNU/Linux. For me the modernish tests never successfully ran. Before #5 it would crash early and most of the tests didn't even get a chance to fail. Unfortunately the install script does not except the shell to just crash and does not report it as a problem. But if you run the tests directly, you should be able to observe how many tests actually run.

The hack in #21 fixes the following tests:

  • shellquote:001
  • shellquote:002
  • shellquote:003
  • shellquote:004

Unfortunately without the fix these get stuck in an infinite loop, blocking the other tests from running. Sadly modernish does not have a way to skip tests, the best I can think of is asking for every other test suite to run:

smoosh modernish/bin/modernish --test -t arith/insubshell/io/is/isset/local/loop_cond/mapr/match/posparam/posparam_spc/@sanitychecks/stack/string/trap/unexport/util

My pull requests should fix every other failure, expect for these four:

  • mapr:004 -- this dies in a stack overflow because the input text is too long and some operation blows up trying to do something recursive on a char list.
  • match:016, match:018, match:019 -- these check (or depend on) escaping the closing bracket in bracket expressions (e.g.: [abc\]], which IMHO is a non-standard feature.

tucak avatar Jun 03 '20 19:06 tucak

Hmm... interesting. In #34 I fix a bug with calls to pipe that was causing smoosh modernish/bin/modernish --test to abort early. When I run modernish locally, I only get the one expected failure (due to control codes).

I suspect there are some platform differences that are affecting Modernish's behavior, and some of those differences are leading to different tests being run... and so exposing different failures. I feel like Travis is going to be the best source of truth here, as annoying as that is.

I semi-relatedly wonder about updating Modernish---my fork is more than 400 commits behind!

mgree avatar Jun 03 '20 21:06 mgree