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

feat: add spectron startup test #730

Merged
merged 26 commits into from
Feb 4, 2019
Merged

feat: add spectron startup test #730

merged 26 commits into from
Feb 4, 2019

Conversation

olizilla
Copy link
Member

@olizilla olizilla commented Dec 5, 2018

This PR adds a basic (the text book) spectron test, modified just enough to make it work with our app

Right now our app won't close after testing, and I have narrowed it down to the way that we keep the webui window active in
https://github.com/ipfs-shipyard/ipfs-desktop/blob/f06cc5919682ba80009bceb07c39a210dfd63e66/src/hooks/webui/index.js#L29-L33

This seems to cause the call to app.stop() in the spectron api to fail. To fix this and more importantly to be kind on users system resources, i think we should avoid keeping the webui window running. I think we should open it when the app is first run, and destroy it when the user closes the window, and create a new one if they choose to open it again. This will require refactoring how we handle the config toggle, as it expects to be able to pull the window from the context. I think we should decouple that.

TODO:

  • DO LATER Refactor webui to create and destroy windows. Refactor things that depend on finding a stable webuiWindow property on the context to not do that.
  • Assert more interesting things about the windows rather than just "do the exist"
  • Tests depend on the babel'd output, so we should either stop using features that require babel in our app code, or run babel before running the tests, and and a watch mode.

fsdiogo and others added 2 commits December 3, 2018 15:00
The app wont shutdown due to how we keep the webui window open.
We don't want to consume system resources keeping multiple
windows open, so we need to refactor things to not expect a
stable webui window on the context.

License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
@ghost ghost assigned olizilla Dec 5, 2018
@ghost ghost added the in progress label Dec 5, 2018
@olizilla olizilla mentioned this pull request Dec 5, 2018
@hacdias
Copy link
Member

hacdias commented Dec 5, 2018

@olizilla I pushed a small fix to master and then merged it onto this branch. It fixes the issue where the app wouldn't close on app.stop.

src/index.js Outdated Show resolved Hide resolved
src/menubar/index.js Outdated Show resolved Hide resolved
@hacdias
Copy link
Member

hacdias commented Dec 12, 2018

So, let's take a look of what's missing. By injecting IPFS_PATH in the environment, we may be able to try out this startup scenarios:

  • No previous repository;
  • Existent okay repository;
  • Existent not-okay (with api) file repository;
  • Make tests work with Babel;

@olizilla anything missing?

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
Copy link
Member

hacdias commented Dec 19, 2018

@olizilla @fsdiogo I created some tests but they only seem to be passing on macOS. They work locally on Windows. I followed the instructions for Spectron on Travis and AppVeyor but it still doesn't seem to work. I will AFK for the next couple of hours. It would be awesome if you could take a look at this please.

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
Copy link
Member

hacdias commented Feb 1, 2019

Just an update: I just updated the tests and they now also mock the home directory so they can be safely run at your computer and they won't change your configurations, nor your IPFS Desktop settings.

On Windows, it seems to be opening tons of empty command line windows and it makes the tests hang. I have to manually click 'Quit' on IPFS Desktop to finish each test manually. Here's a related issue on Spectron: electron-userland/spectron#60.

Despite this issue, they seem to be testing what they should test. I'm waiting for the CI to see how they behave on other OSes. Although I'm expecting them to fail on Linux (as they were before) and on Windows (due to the command line windows...). Hopefully macOS tests will pass, as they were before.

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
Copy link
Member

hacdias commented Feb 1, 2019

One more update: I noticed the reason for tests to be failing is not the daemon, is not spectron. It's the Web UI window. Most of the times it can't quit so I decided to remove the Window completely and now the tests pass... This might also be related to the issue @lidel has reported which required multiple clicks on 'Quit' to close IPFS Desktop (#781).

In other "news", I decided to allow failures on Linux because of two reasons: only OS where the tests don't pass and we are setting Linux support as experimental since it doesn't work well on all desktop environments: #788 (comment).

/cc @ipfs-shipyard/gui

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
Copy link
Member

hacdias commented Feb 1, 2019

I managed to fix this in the meanwhile - and actually I don't think it's related to #781 anymore - by setting the node integrations to true on testing environment. I don't yet understand why is it needed but hey! The tests pass.

Ref.: electron-userland/spectron#347

On a brighter side, we now have tests for the most important startup scenarios!

@hacdias hacdias changed the title wip: add spectron startup test feat. add spectron startup test Feb 1, 2019
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias hacdias changed the title feat. add spectron startup test feat: add spectron startup test Feb 1, 2019
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias hacdias requested review from lidel and fsdiogo February 1, 2019 15:46
@hacdias
Copy link
Member

hacdias commented Feb 1, 2019

@olizilla @fsdiogo @lidel could you take a look at this please? Perhaps we could merge this!

@hacdias
Copy link
Member

hacdias commented Feb 4, 2019

@olizilla I'm merging this tests so we can work on other tests on top of these. They're passing and look good. We can change anything later if needed.

@hacdias hacdias merged commit 66827d1 into master Feb 4, 2019
@hacdias hacdias deleted the feat/add-electron-tests branch February 4, 2019 10:54
@ghost ghost removed the in progress label Feb 4, 2019
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.

3 participants