search

myst-lang / myst

A structured, dynamic, general-purpose language.
http://myst-lang.org
MIT License
119 stars 17 forks source link

Hide warning output when running specs #184

Closed faultyserver closed 6 years ago

faultyserver commented 6 years ago
screen shot 2018-04-07 at 7 31 45 pm

Parser warnings that come up from various specs have been appearing in the output for a while now. I think they used to be hidden by a MYST_ENV variable being set to test, but that isn't happening anymore.

The proper solution for this is to use interpret_with_mocked_output for these specs so that the output is captured and can be tested properly (right now the specs just check if a warning was raised, not what that warning was).

Most of the offending specs are probably using the it_warns helper method defined here: https://github.com/myst-lang/myst/blob/d5015522e4773429e578de347b93eabbe6a34c1b/spec/support/interpret.cr#L76-L83

The fix would be to call interpret_with_mocked_output(source) from this method instead of creating a new Interpreter and running it directly, then checking the warnings from the interpreter object returned by that method.

faultyserver commented 6 years ago

It actually looks like these messages are coming from vm_spec.cr where it's testing the standard IO usage. I'm not sure what the best approach to solving this is.

Jens0512 commented 6 years ago

What makes you think it comes from vm_spec.cr? Code containing underscore vars is not run in the VM specs. And we really should rename VM 😅 ... I clearly remember dealing with just this issue before, I might have forgotten to push it here though, or I i did, and it somehow got broken.

faultyserver commented 6 years ago

Well, I commented out everything in vm_spec because a test was failing (I had made some radical changes at the time) and the warning output didn't appear.

I think this is the offending line:

https://github.com/myst-lang/myst/blob/d5015522e4773429e578de347b93eabbe6a34c1b/src/myst/vm.cr#L15

It assigns the ENV variable, which will persist for the duration of the test suite. So even if the tests that actually output warnings to the terminal are in another file, this should be addressed to avoid leaking.

I think the best solution would be to remove that line entirely. The VM shouldn't be responsible for managing environment variables.