Skip to content
This repository has been archived by the owner on Dec 18, 2020. It is now read-only.

add basic electron test #46

Merged
merged 7 commits into from
May 26, 2020
Merged

add basic electron test #46

merged 7 commits into from
May 26, 2020

Conversation

pinussilvestrus
Copy link

To be rebased with #43


Currently not working

  • on Linux
  • do not closing electron instance after tests on macOS

@pinussilvestrus pinussilvestrus force-pushed the unit-tests branch 2 times, most recently from 03de3bc to e42175e Compare March 8, 2020 11:45
package.json Outdated Show resolved Hide resolved
@pinussilvestrus
Copy link
Author

I would love to see that one merged to start testing additional Electron functionality, e.g. introduced with #100 ... Still, some stuff to be fixed (especially on Linux, but I have no device to test that locally).

@shrirambalaji
Copy link
Collaborator

shrirambalaji commented May 22, 2020

@pinussilvestrus I can help out with unit tests on Linux, as that's my daily driver.

I'm not very clear on what needs to be tested here though. Would be great if you could point me on what to test.

@pinussilvestrus
Copy link
Author

Thanks for your help @Shriram-Balaji !

Its basically this failing test case.

This PRs only aims to add a basic test suite with a simple starting test, which currently fails on Linux.

@shrirambalaji
Copy link
Collaborator

shrirambalaji commented May 23, 2020

@pinussilvestrus After some hiccups, I've got the unit tests working. So the tests were not working on CI/CD because when Spectron spins up a Chrome Instance to run the Selenium WebDriver, the internal ChromeDriver looks for an actual GUI.

In server environments, we don't have a GUI and hence have to use Xvfb which emulates an actual X-Window Frame Buffer, using virtual memory. NOTE: This applies only to Linux, for Windows and macOS this is not applicable.

I've also done some renaming to the scripts, because we have both lint and tests now.

@shrirambalaji
Copy link
Collaborator

shrirambalaji commented May 24, 2020

@pinussilvestrus Also, do you think we should use jest instead of mocha, to keep it consistent with excalidraw web?

@pinussilvestrus
Copy link
Author

Cool stuff 👏

Also, do you think we should use jest instead of mocha, to keep it consistent with excalidraw web?

makes absolute sense I guess

@pinussilvestrus
Copy link
Author

This applies only to Linux, for Windows and macOS this is not applicable.

.. but we should ensure tests work properly on mac and windows, too. Which they did before / without adding the xvfb support, am I right?

@shrirambalaji
Copy link
Collaborator

shrirambalaji commented May 24, 2020

This applies only to Linux, for Windows and macOS this is not applicable.

.. but we should ensure tests work properly on mac and windows, too.

Yes, we should add test workflows for macOS and Windows too. For Windows - there's a "screen" available to run even in headless mode, and works on CI. For macOS, I can't seem to find the right documentation, but in theory, it should work without us needing to install / configure Xvfb.

Which they did before / without adding the xvfb support, am I right?

Well no, the tests were run against only the ubuntu-latest image on github actions, and there were no workflows for Windows / Mac ( ie. we were actually not running tests for them). There are build steps, but not the lint workflow if im not wrong.

@shrirambalaji
Copy link
Collaborator

shrirambalaji commented May 24, 2020

@pinussilvestrus So I'm still running into the same errors for both macOS and Windows (Added separate workflows to run tests on em') . Could you try running the tests on your local machine, and check if the Mac tests work? Afaik they should work.

@pinussilvestrus
Copy link
Author

I have currently the problem to install spectron correctly

image

Do you have an idea?

@shrirambalaji
Copy link
Collaborator

shrirambalaji commented May 24, 2020

@pinussilvestrus Can you try yarn install --force? I experienced issues with spectron as well, when I bumped the version.

@pinussilvestrus
Copy link
Author

It does not work for me unfortunately. Maybe I can figure out a fix for that.

@shrirambalaji
Copy link
Collaborator

shrirambalaji commented May 25, 2020

It does not work for me unfortunately. Maybe I can figure out a fix for that.

Hmm, any luck with it @pinussilvestrus? Can you check if this works for you? I doubt that it would, but worth a try I guess. The only other option I can think of is to remove node_modules and retry, but I think you might have already checked that.

Looks like there's some issue of spectron being compatible with electron v9, electron-userland/spectron#610. The error reported seems to be on Mac, not sure if it's related.

I think it might be worth it to downgrade electron to v8.x, considering that it might be more stable.

@shrirambalaji shrirambalaji force-pushed the unit-tests branch 3 times, most recently from d18fb69 to 787def4 Compare May 25, 2020 19:01
 * Add test:spec script
* Replace test:code with lint:code
* Update lockfile
* Increase mocha timeout
* Update electronBinary path
* Runs both lint, test for multiple platforms
* WIP: Disable test-windows workflow
@shrirambalaji
Copy link
Collaborator

@pinussilvestrus I have downgraded electron to 8.3.0, and along with that have gotten the tests running on both macos and linux. Also, I'm guessing if you clear node_modules, spectron should install on your local system now.

But, I can't seem to clearly figure out why, windows is not working. Would it be ok for us to add the windows test workflow later?

@shrirambalaji shrirambalaji marked this pull request as ready for review May 25, 2020 20:17
@pinussilvestrus
Copy link
Author

I think that's ok for now. Can you maybe add an issue for adding windows unit test support as a next step? Then we would be ok with merging this one

@shrirambalaji
Copy link
Collaborator

I think that's ok for now. Can you maybe add an issue for adding windows unit test support as a next step? Then we would be ok with merging this one

Just created an issue for that #163

@pinussilvestrus pinussilvestrus merged commit 6138f95 into master May 26, 2020
@pinussilvestrus pinussilvestrus deleted the unit-tests branch May 26, 2020 06:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants