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

Integrate spectron (UPLOAD-440) #1360

Open
wants to merge 47 commits into
base: master
Choose a base branch
from
Open

Integrate spectron (UPLOAD-440) #1360

wants to merge 47 commits into from

Conversation

ginnyyadav
Copy link
Member

Simple spectron integration with a very simple test example so we can build off of it.

@ginnyyadav ginnyyadav requested review from gniezen and tjotala January 26, 2021 20:30
Copy link
Member

@gniezen gniezen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome! I've added a couple of suggestions and comments, but nothing major. Code quality is 💯

@@ -36,6 +36,7 @@ jobs:
# Test
- run: yarn lint
- run: yarn test
- run: yarn run spectron-test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- run: yarn run spectron-test
- run: yarn spectron-test

appveyor.yml Outdated
@@ -35,6 +35,7 @@ test_script:
- node --version
- yarn lint
- yarn test
- yarn run spectron-test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- yarn run spectron-test
- yarn spectron-test

package.json Outdated
@@ -31,7 +31,8 @@
"package-win": "npm run build && electron-builder --win --x64",
"package-mac": "npm run build && electron-builder --mac",
"package-linux": "npm run build && electron-builder --linux",
"package-all": "npm run build && electron-builder -mwl"
"package-all": "npm run build && electron-builder -mwl",
"spectron-test": "yarn build && mocha -r esm"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've migrated from mocha to jest a while ago. I'm just wondering whether the spectron tests will work with jest, or is it only possible with mocha?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops, didn't realize that, totally will work with jest, will make it so!

await stopApp(app);
});

it('should open', async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually async? I don't see an await.

await LoginScreen.uploaderLogo.getText()
.should.eventually.equal('Uploader');
await LoginScreen.forgotPasswordLink.should.eventually.exist;
await LoginScreen.supportLink.getText()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember we had an issue for a while where the support link wasn't working. Can we also add tests here to check that the URLs don't 404?


global.before(() => {
should();
use(chaiAsPromised);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind adding a comment here to explain why chaiAsPromised is necessary here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this won't be here if we use jest i think.

@ginnyyadav ginnyyadav requested a review from gniezen February 3, 2021 22:26
@ginnyyadav
Copy link
Member Author

ginnyyadav commented Feb 3, 2021

Went ahead and switched to jest...a couple things here..

  • i made stricter checking on the URLs, but i can't go any further than that since they open in a chrome browser and not in the electron app. the positive side of this is that with the web nightwatch tests we can make sure these specific urls are not returning 404s.

  • I had to put in an explicit wait since the login button on uploader on the windows machine is loading enough for the automation to click on it and render it as visible, but not loading completely enough to actually complete the function of logging in

  • I've put in a remote session for debugging. the password is in the 1 password vault. I did this for debugging but not sure if you want me to leave it in there in case anyone else finds this helpful. If you'd like me to remove it I can

special thanks to @krystophv and @gniezen for help on this

Copy link
Member

@gniezen gniezen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, great job @ginnyyadav ! I just have two comments:

  • run is not necessary when using yarn
  • Please remove the app/package-lock.json file, as that as created by npm.

README.md Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@gniezen gniezen changed the title Integrate spectron Integrate spectron (UPLOAD-440) Feb 8, 2021
gniezen
gniezen previously approved these changes Feb 8, 2021
Copy link
Member

@gniezen gniezen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! :shipit:

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