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

Prevent console.error from going off because it causes some problems in test code #85

Merged
merged 5 commits into from
Oct 28, 2021

Conversation

timotree3
Copy link
Contributor

For example, this CI run fails to due to some getFileName error which I think is caused by the usage of console.error

@timotree3
Copy link
Contributor Author

timotree3 commented Oct 27, 2021

I may have merged #79 prematurely. When running tests on develop, I saw that 0 out of 0 tests passed, which counts as success :/ This PR now fixes that and additionally updates the lints to apply to the tests/ folder

@timotree3
Copy link
Contributor Author

I actually think jest is pointing us to a valid criticism of holochain-conductor-api. I don't think libraries should log things, and they should definitely not log errors under expected and normal circumstances. If we're looking for the proper solution here, I propose that we propagate this error properly. Perhaps a good solution would be to capture this error but not print it, and then edit the error path of AppWebsocket.connect to detect this error, and include it. e.g.

Exception: Could not connect to holochain app websocket at localhost:xxxx
Note: This may be because we are in a launcher environment and failed to detect that. Reason: ${launcherDetectionError}

@guillemcordoba
Copy link
Contributor

guillemcordoba commented Oct 28, 2021

So the problem is really that we can't hide the error in the browser, see https://stackoverflow.com/questions/12915582/eliminate-404-url-error-in-console and verlok/vanilla-lazyload#460. When we do the fetch from a browser but not inside the launcher environment, we are going to get an error in the console, and we can't do anything for that. To avoid people being confused about that, I included the console.warn just after that to let the devs know that they don't have to worrry about that 404 error.

What we could do is do the fetch in the first AppWebsocket.connect(), and then it's not a side effect from importing the library... I like that more, but it still has the same problem with the 404 error, we can't really capture it.

Would you be okey if I did that change?

The thing you are running into is that Jest is not really a browser environment, so it shouldn't even do the fetch, but Jest tries to simulate a browser so we have thing like window set. Jest is weird...

@timotree3
Copy link
Contributor Author

I could imagine wanting to test launcher autodetection using jest. maybe the best approach would be to revert to before this PR but just put the console.warn and console.error within an if that we are not inside jest?

@timotree3
Copy link
Contributor Author

I will test out this version as requested.

@timotree3 timotree3 merged commit bc23935 into develop Oct 28, 2021
@timotree3 timotree3 deleted the fix-launcher-check-in-jest branch October 28, 2021 18:12
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