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

Issues with Weather Tests. - Tests currently disabled. #1840

Closed
MichMich opened this issue Dec 29, 2019 · 16 comments
Closed

Issues with Weather Tests. - Tests currently disabled. #1840

MichMich opened this issue Dec 29, 2019 · 16 comments

Comments

@MichMich
Copy link
Collaborator

MichMich commented Dec 29, 2019

For some unknown reason most of the weather module tests seem to fail (both on Travis as well as local) due to timeout issues. To be able to continue the development I have disabled the weather tests by adding a skip statement: https://github.com/MichMich/MagicMirror/blob/develop/tests/e2e/modules/weather_spec.js#L18

This means the weather tests are currently not used. This could impact the quality of the code. I can't seem to find any reason why these tests time out.

If anyone is able to solve this issue and get travis to successfully complete the tests, please send a PR. Your contribution would be highly appreciated.

Thanks!

@MichMich MichMich changed the title Issues with Weather Tests. Issues with Weather Tests. - Tests currently disabled. Dec 29, 2019
@MichMich MichMich pinned this issue Dec 29, 2019
@roramirez
Copy link
Contributor

After merge #1849 I could check it

@MichMich
Copy link
Collaborator Author

MichMich commented Jan 2, 2020

Fixed by reverting electron. See #1800

@MichMich MichMich closed this as completed Jan 2, 2020
@MichMich MichMich unpinned this issue Jan 2, 2020
@sdetweil
Copy link
Collaborator

sdetweil commented Jan 3, 2020

still failing today
see pr #1858

@MichMich
Copy link
Collaborator Author

MichMich commented Jan 3, 2020

They run ok now. They timeout after 10sec. We might need to stretch that 10 sec.

@sdetweil
Copy link
Collaborator

sdetweil commented Jan 3, 2020

The code says if under CI then timeout is 30 seconds

@MichMich
Copy link
Collaborator Author

MichMich commented Jan 3, 2020

@sdetweil
Copy link
Collaborator

sdetweil commented Jan 3, 2020

Ok. Other tests call a function in global_setup to set timeouts. Figured they all would

@roramirez
Copy link
Contributor

I've try catch the failed of this and apparently is because is too slow to run these tests. There a other line
https://github.com/MichMich/MagicMirror/blob/69a887fb05af79071eb20875c1c4a856627f7811/tests/e2e/modules/weather_spec.js#L11
for a wait. Maybe @fewieden can check a look of this.

@fewieden
Copy link
Contributor

@roramirez @MichMich I tried this morning the newest version of develop and run the tests locally, they all passed after ~7s.

What is the setup I need to reproduce the issue?

Since this seems to be still relevant, we should reopen the issue.

@roramirez
Copy link
Contributor

Hi @fewieden

You should try into a similar environment where is running like Travis. Maybe you can using the follow guide (I'm use this one) https://jonlabelle.com/snippets/view/markdown/run-travis-ci-builds-locally-with-docker.

Or also requests the debug mode for your Fork in Travis
https://docs.travis-ci.com/user/running-build-in-debug-mode/#enabling-debug-mode

Since this seems to be still relevant, we should reopen the issue.

Yes. Should reopen this issue

@roramirez
Copy link
Contributor

@fewieden I forget other thing. Also should run the test against the PR #1840

@fewieden
Copy link
Contributor

fewieden commented Feb 2, 2020

@roramirez the PR that you tried to reference is a link to this issue.

Therefore I tried https://jonlabelle.com/snippets/view/markdown/run-travis-ci-builds-locally-with-docker to debug this commit which had the original build log.

Executing the tests inside the docker container resulted in a different error

Capture

What I found is https://stackoverflow.com/questions/50642308/webdriverexception-unknown-error-devtoolsactiveport-file-doesnt-exist-while-t but even with adding all those argumens to the chrome driver the issue still appeared. Maybe I passed them wrong?

Running the tests locally gives a third result. Sometimes they pass, sometimes the same tests fail. I could see that they made actual requests to the api server instead of using the mock. Maybe we have a timing iue in there.

Updating electron and spectron to their latest stable versions is passing all the tests. What is failing, is the npm list command afterwards.

Capture

CC @MichMich

@roramirez
Copy link
Contributor

Hi @fewieden You're right. I've reference wrong the PR. This should be #1892. Sorry for that. This change is already merge in develop you could try against this branch. The test for weather is disabled in this branch.

The problem you are getting is because the sandbox mode is enable by default. More information about this is #1892

Setting the enviroment variable ELECTRON_DISABLE_SANDBOX=1 before run the test should be enough.

@fewieden
Copy link
Contributor

@roramirez looks like I found the issue

@roramirez
Copy link
Contributor

@fewieden Nice work!. Seems there this was little trickle but you found it.

@MichMich You should merge the #1927

@MichMich
Copy link
Collaborator Author

Merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants