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

Fix CI #1849

Merged
merged 3 commits into from
Jan 1, 2020
Merged

Fix CI #1849

merged 3 commits into from
Jan 1, 2020

Conversation

roramirez
Copy link
Contributor

Revert commit introduced in #1827. The change introduce a failure for all e2e tests.

This change revert the commit 2fbedca

--
Happy new year ("20".repeat(2))

@sdetweil
Copy link
Collaborator

sdetweil commented Dec 31, 2019

NO.. we need the helper fix.. need to fix CI some other way..
the package-lock.json was not updated for this fix...

so maybe travis doesn't have the module loaded...

@sdetweil
Copy link
Collaborator

the vendor tests still fail.

@MichMich
Copy link
Collaborator

MichMich commented Jan 1, 2020

@sdetweil I Will test this PR tonight. If this fixes the issue I’ll merge it since we need Travis to complete successfully in order to release 2.10 today (according to release schedule).

@sdetweil
Copy link
Collaborator

sdetweil commented Jan 1, 2020

How do we fix the testing cache to have the helper in the main node_modules instead the modules/node_modules?

@MichMich
Copy link
Collaborator

MichMich commented Jan 1, 2020

Will try tonight. Don’t want to have the helper in an external repo anyway. Might need to push that change onto the next release.

@sdetweil
Copy link
Collaborator

sdetweil commented Jan 1, 2020

I think the helper being erased is a big problem. I think it is a security fix that is erasing. If modules not listed but installed, they are removed

@MichMich
Copy link
Collaborator

MichMich commented Jan 1, 2020 via email

@sdetweil
Copy link
Collaborator

sdetweil commented Jan 1, 2020 via email

@MichMich
Copy link
Collaborator

MichMich commented Jan 1, 2020

Reading that, I’m not confident it was caused by an NPM install. At at least not an non install ran in the correct folder. But non the less I will try to look for a solution. First priority is to get Travis to work again.

@sdetweil
Copy link
Collaborator

sdetweil commented Jan 1, 2020 via email

@sdetweil
Copy link
Collaborator

sdetweil commented Jan 1, 2020 via email

@MichMich
Copy link
Collaborator

MichMich commented Jan 1, 2020

I can't release without Travis. Releases are blocked. And for a good reason. Will work on a solution now.

@MichMich MichMich merged commit d2b30b4 into MagicMirrorOrg:develop Jan 1, 2020
@MichMich
Copy link
Collaborator

MichMich commented Jan 1, 2020

@sdetweil the node helper is no longer part op de node_modules folder. Now looking for the weather tests fails.

@roramirez
Copy link
Contributor Author

I think the helper being erased is a big problem. I think it is a security fix that is erasing. If modules not listed but installed, they are removed

I can understand why will be the better if we has a node helper in external repository and what kind of issue could fix include in this way. We should keep this code in main code of project, its to more easy maintent this code in this repo.

@sdetweil
Copy link
Collaborator

sdetweil commented Jan 1, 2020

I asked multiple times for Mich to create the repo on his GitHub, but got no action. So I created it on mine.

Mich is welcome to move it to his.
I am trying to stop user acceptance problems

@MichMich
Copy link
Collaborator

MichMich commented Jan 1, 2020

This is already fixed. The node helper is already outside the modules folder (in /js to be exactly). It is aliased so you can just use the same require statement. (Non breaking).

I want to keep everything in one repo to keep it simple. This accomplishes that.

@sdetweil
Copy link
Collaborator

sdetweil commented Jan 1, 2020

As for the Travis problem there are electron version dependencies on some of the libraries used by the test cases. Spectron, for example.
I updated that one to match the electron version.
Don't know if there are other lib problems too.

Tests run locally after install work great.(weather works, vendor still fails, but still reports ok)
So how do we get those libs updated in Travis environment?

There is no change mgmt guidance for the Travis tests

@sdetweil
Copy link
Collaborator

sdetweil commented Jan 1, 2020

Helper in js. good! I didn't see that as a possibility.
The alias, will have to see how u did that

@roramirez
Copy link
Contributor Author

As for the Travis problem there are electron version dependencies on some of the libraries used by the test cases. Spectron, for example.
I updated that one to match the electron version.
Don't know if there are other lib problems too.

Tests run locally after install work great.(weather works, vendor still fails, but still reports ok)
So how do we get those libs updated in Travis environment?

There is no change mgmt guidance for the Travis tests

What O.S, npm and node versions are you using?

Could you paste here the package.json in your local environment?

Also, npm list

@sdetweil
Copy link
Collaborator

sdetweil commented Jan 1, 2020

Will do.
node -v = v10.18.0
npm -v =6.13.4
Tried on raspian, Ubuntu,

All tests fail on macOs Catalina. 10.15.2

I am away for most of the rest of the day
I posted a new package-lock with this pr. which solved the problem originally (Travis went from failing to passing)

I did a clone, checkout for develop, npm install, then ran two testcase sets

npm list
package.json
package-lock.json

lsb_release -a

No LSB modules are available.
Distributor ID: Ubuntu
Description: Ubuntu 18.04.3 LTS
Release: 18.04
Codename: bionic

@sdetweil
Copy link
Collaborator

sdetweil commented Jan 2, 2020

looks like the screen setup for travis has changed

https://electronjs.org/docs/tutorial/testing-on-headless-ci

when I do export DISPLAY=:0
and then run the npm run test:e2e (locally) it works
but not the vendor tests..

the vendor tests are because the port didn't get set when using the js/electron.js launch
I added debug to the vendor_spec.js and here is the output of the before, launching js/electron.js
the port is not set

Vendors
starting js/electron
electron path=/home/sam/MagicMirror/node_modules/.bin/electron
new electron app =[object Object]
parms={"host":"127.0.0.1","port":9515,"quitTimeout":1000,"startTimeout":5000,"waitTimeout":5000,"connectionRetryCount":10,"connectionRetryTimeout":30000,"nodePath":"/usr/bin/node","path":"/home/sam/MagicMirror/node_modules/.bin/electron","args":["/home/sam/MagicMirror/tests/e2e/../../js/electron.js"],"chromeDriverArgs":[],"env":{},"workingDirectory":"/home/sam/MagicMirror","webdriverOptions":{},"requireName":"require"}

if I change the get urls to the 9515 port, the it works..

this didn't work (tests/e2e/from vendor_spec.js)
describe("Get list vendors", function () {

	before(function () {
		process.env.MM_CONFIG_FILE = "tests/configs/env.js";  // < ----
	});

that before() is never called.
and if u move the process.env. to before the app.start() it doesn't work there either..

@sdetweil
Copy link
Collaborator

sdetweil commented Jan 2, 2020

the untouched e2e test also works on macOS catalina after doing export DISPLAY=:0
i had to change the address for the vedor tests to port 9515, but it worked then

@roramirez roramirez deleted the fix-ci branch January 20, 2020 22:08
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