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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mgree
Copy link
Owner

@mgree mgree commented Jun 3, 2020

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

@tucak
Copy link
Contributor

tucak commented Jun 3, 2020

I checked it locally, it does not break anything. Standing on 6156f8f modernish runs 116 tests and then is killed because of the issue #20 fixed. Standing on 84cb012 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

@mgree
Copy link
Owner Author

mgree commented Jun 3, 2020

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!

@tucak
Copy link
Contributor

tucak commented Jun 3, 2020

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.

@mgree
Copy link
Owner Author

mgree commented Jun 3, 2020

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!

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.

2 participants