zulip / zulip-mobile

Zulip mobile apps for Android and iOS.
https://zulip.com/apps/
Apache License 2.0
1.29k stars 651 forks source link

npm run test not working #3178

Open mbuotidem opened 5 years ago

mbuotidem commented 5 years ago

I just cloned the latest version of zulip-mobile to work on it and after setting everything up (I.e getting it to run on my android) I decided to run 'npm run test' before making any changes. However, the command does not run and states "Windows Subsystem for Linux has no installed distributions.

I found this weird because I've successfully run the test before so I restored the old version of zulip-mobile from my Recycle bin, moved it to my Desktop and tried to run the test. It ran without issues. Not sure what I am missing here.

Please see attached screenshot with both cmd windows running side by side.

zulip npm run test
borisyankov commented 5 years ago

@mbuotidem we did implement some changes to how the test run just yesterday. The changes were Unix-dependent but we took some steps to try to keep it Windows-compatible.

We were discussing how important it is to keep the Windows compatibility, but considering we got this feedback so fast from you, I think this should be a high priority to keep it Windows-compatible, possibly implementing our core utils in Node and not Bash.

@gnprice will be giving more details on our thoughts and next steps on this.

gnprice commented 5 years ago

Hi @mbuotidem ! Thanks for the report. That sure is a strange error message for Windows to give you there.

We switched our scripts yesterday (in 6c25beeb0) to run on Bash, which is a major improvement over cmd.exe that lets us automate simple things (like testing only for files that have changed, which makes running tests much much faster) very easily.

You should already have Bash installed -- it comes with Git for Windows, because Git relies on it. So let's sort out how to fix this configuration for you, and then we'll adjust our config and/or our setup instructions so it works easily for other Windows users too.

It looks like the error you're getting is that

Would you try these instructions to set an absolute path for the package.json scripts to use? https://stackoverflow.com/a/46006249/378130 Let us know exactly what command you ended up running, and what the results are.

mbuotidem commented 5 years ago

@gnprice Happy to help. Before trying these commands I decided to install WSL and see what difference that would make, if any. Here is the result of that effort.

  1. From Windows cmd

npm run test after wsl install

  1. From Git Bash on Windows:

git bash wsl on

  1. From WSL on a WSL specific zulip-mobile folder

wsl

I am now going to remove WSL and try the instructions in the link you posted.

gnprice commented 5 years ago

Thanks @mbuotidem ! I will be curious to hear back on how the instructions I posted go for you. :-)


Separately, I am interested in getting a good set of instructions for setting up the dev environment on WSL. We actually have a rough version already: https://github.com/zulip/zulip-mobile/blob/master/docs/howto/windows.md

If you're interested in WSL, I would love to have you try those setup instructions out and debug any steps that don't work.

With whatever setup you did the other day, it seems like you got fairly close!

  • yarn run test:lint also failed WHILE yarn run jest and yarn run test:prettier succeeded.

The output you posted looks like success to me. What information do you have in mind that suggests it failed?

If that was a success, then the WSL picture is looking pretty good. There's that one failure with the flow server... but you had the same failure on cmd.exe, and that's one spot where we haven't really changed anything (the command inside is just flow, nothing more), so I wonder if that's an unrelated issue in your setup.

For debugging the Flow issue, I'd be curious about the following commands:

yarn run flow
node_modules/.bin/flow

Do those work, or do they give the same error?

And: note those lines like "Logs will go to /tmp/flow/zSmntzS...", and "Monitor logs will go to ...". I'd be interested in seeing the contents of those log files -- those may have some good information on the cause of the issue.

mbuotidem commented 5 years ago

@gnprice, It is certainly possible that there might be an unrelated issue in my setup, I will be exploring running these tests in WSL on a different machine. But for now, I forged ahead with troubleshooting this on my main machine.

I've attached some the log files from running yarn run flow and node_modules/.bin/flow in WSL. Both commands failed with the same error. The log files from both errors were pretty much word for words apart from the datetime so I only attached one set.

zSmntzSczSUserszSMichaelzSzZulip-mobile.log.txt zSmntzSczSUserszSMichaelzSzZulip-mobile.monitor_log.txt

Oh and you're right, the yarn run test: lint did not fail so only the flow test needs to be figured out for WSL.

mbuotidem commented 5 years ago

And here are the logs of the same command run from windows cmd prompt.

CzCzBUserszBMichaelzBzZulip-mobile.log.txt CzCzBUserszBMichaelzBzZulip-mobile.monitor_log.txt

mbuotidem commented 5 years ago

@gnprice I figured it out partially today. Since Windows now has WSL, whether a Windows installation has WSL configured or not, the System32 folder now has a bash.exe file. That is why I was getting the message that WSL is not configured on cmd.

To get Windows cmd to run the bash.exe that comes with Git Bash for Windows, the Git Bash path has to be added to to PATH in the System environment variable above the path to the WSL version of bash which is in . C:\WINDOWS\system32. That way, when cmd looks for bash.exe it hits the Git Bash version first.

I now have the test running in cmd via git-bash. Looks like we need two sets of Windows documentation though. One for running on Windows without WSL so that a tid-bit like this can be highlighted, but also the one I worked on for running on Windows with WSL.

Speaking of which, the flow test is still not working on WSL. Tried again today, still having the same "We've run out of filesystems to use for shared memory" error that is in the "zSmntzSczSUserszSMichaelzSzZulip-mobile.log.txt" log file above.

Did some digging and this might be helpful : https://github.com/facebook/flow/issues/2152.

chrisbobbe commented 4 years ago

Looks like we need two sets of Windows documentation though. One for running on Windows without WSL so that a tid-bit like this can be highlighted, but also the one I worked on for running on Windows with WSL.

@ray-kraesig, since it may be fresh in your mind following the Windows build failures a little while ago, do you know if we still need to document Windows development without WSL? My only context for this is the quote above and the fact that this issue is still open; I haven't investigated further. And the current doc.

rk-for-zulip commented 4 years ago

Well, we still do. Whether or not we need to is a subtly different kettle of fish.

I lean towards "yes": setting up and using WSL seems to still be a complicated enough affair that, if we're going to drop ordinary Windows support for the sake of simpler instructions, we'd probably be better off moving to a Docker-image-for-development strategy like the server. (I haven't actually investigated that route, though, so take this with a grain of salt.)

Considered strictly as a documentation issue, this is closed by #3779. Considered as a build issue, however, it's a close relative of #3776, and will probably be closed by the same action (whatever that ends up being).

chrisbobbe commented 4 years ago

Well, we still do.

Ah, I see now. There may still be a tweak that could help the docs: I saw the filename of windows.md and assumed this was the entire doc for developing using Windows. And, since that doc doesn't have a heading as large as its first one, "Developing on Windows using WSL," that describes working without WSL, I assumed there was no documentation anywhere for non-WSL development. (Caveat lector: I have not done development on Windows, with or without WSL, and I don't really know what WSL is.)

Sounds like there's still a build issue that's keeping this issue open, which is fine.

I wonder about a) changing the filename windows.md to something like windows-using-wsl.md, and b) highlighting the "Using WSL is not a requirement for working on Zulip Mobile..." paragraph so that it's at least as visible as the main header, maybe with some alert formatting, or just by moving it above the header (and, perhaps, prepending "Note:" to it).