Migrate clicktests from nightmare to puppeteer #1336
Conversation
| @@ -27,6 +27,7 @@ addons: | |||
| apt: | |||
| packages: | |||
| - xvfb | |||
| - libgbm1 | |||
There was a problem hiding this comment.
| @@ -69,7 +69,7 @@ module.exports = (grunt) => { | |||
| options: { | |||
| reporter: 'spec', | |||
| require: './source/utils/winston.js', | |||
| timeout: 15000, | |||
| timeout: 35000, | |||
There was a problem hiding this comment.
puppeteer default timeout is 30000ms.
There was a problem hiding this comment.
Would the default be enough?
There was a problem hiding this comment.
Yes, I also could change the timeout to 15000 which we used before, but opted for sticking with the puppeteer defaults, that's why I have increased it here.
There was a problem hiding this comment.
Ah, this is the mocha timeout.
I thought this is setting the puppeteer timeout to 35s instead of 30s and was wondering whether not setting it would be fine.
| @@ -153,6 +153,7 @@ module.exports = (grunt) => { | |||
| }, | |||
| mocha: { | |||
| options: { | |||
| esversion: 8, | |||
There was a problem hiding this comment.
Why does JSHint have esversion: 6?
Does it not support anything higher?
There was a problem hiding this comment.
Because that is what esnext was before https://jshint.com/docs/options/#esnext.
There was a problem hiding this comment.
Makes sense, but can we set it to esversion: 8 for the test files (and node files)?
There was a problem hiding this comment.
Yes, we will but not as part of this PR.
| @@ -10,7 +10,7 @@ <h4 class="stash-toggle-text" data-bind="click: toggleShowStash"> | |||
| </h4> | |||
| <div class="list-group" data-bind="foreach: stashedChanges"> | |||
| <div class="list-group-item"> | |||
| <a href="#" class="stash-apply octicon-circled pull-left bootstrap-tooltip" data-bind="html: applyIcon, click: apply" data-toggle="tooltip" title="Apply this stash"></a> | |||
There was a problem hiding this comment.
The clicktest did fail on macOS because this used floats and couldn't be clicked. It now works by using display: inline-block which is nicer.
nmclicktests/environment.js
Outdated
| width: this.config.viewWidth, | ||
| height: this.config.viewHeight | ||
| }, | ||
| headless: true |
There was a problem hiding this comment.
Change this to false to see the browser!
There was a problem hiding this comment.
Would it makes sense to make that configurable for local runs?
There was a problem hiding this comment.
Yes it would be nice, I use to set a GUI=1 env variable for this
There was a problem hiding this comment.
Yes, we could make them configurable via environment variables and document them because otherwise they are as hidden as this boolean.
I usually don't use environment variables to configure stuff like this, is GUI=1 unique enough? Would it clash with some other programs? What about GUI=true?
nmclicktests/spec.branches.js
Outdated
| .wait(200) | ||
| .type('input.name', '-4\u0028\u000d') | ||
| .wait('[data-ta-name="search-4"]') | ||
| it('search for the hidden branch', async () => { |
There was a problem hiding this comment.
Enabled this test!
| fs.writeFile(req.body.file, content, () => res.json({})); | ||
| fs.writeFileAsync(req.body.file, content) | ||
| .then(() => res.json({})) | ||
| .then(emitWorkingTreeChanged.bind(null, req.body.path)); |
There was a problem hiding this comment.
To make the tests more reliable trigger working tree changes manually.
There was a problem hiding this comment.
I don't particularly like the bind. Would this work too?
| .then(emitWorkingTreeChanged.bind(null, req.body.path)); | |
| .then(() => emitWorkingTreeChanged(req.body.path)); |
There was a problem hiding this comment.
Yes, it would but I just want to keep the existing codestyle for now.
In the future we will replace all of this with async await anyway.
source/git-api.js
Outdated
| }); | ||
| app.post(`${exports.pathPrefix}/testing/git`, ensureAuthenticated, (req, res) => { | ||
| jsonResultOrFailProm(res, gitPromise(req.body.command, req.body.repo)); | ||
| jsonResultOrFailProm(res, gitPromise(req.body.command, req.body.path)); |
There was a problem hiding this comment.
This was the only case where the repro path variable name was repo. I just changed it to path to be consistent with the rest.
| background: #55323C; | ||
| h4 { | ||
| margin-top: 0px; | ||
| } | ||
| margin-bottom: -15px; | ||
|
|
||
| .toggle-show-commit-diffs { |
There was a problem hiding this comment.
is this related to some test failing now?
There was a problem hiding this comment.
No, the stash tests did fail before on macOS which is fixed by this.
The current test failures (mainly on macOS) are different, but I don't have a mac available so they are pretty hard to debug for me.
There was a problem hiding this comment.
Just want to note that macOS tests sometimes succeed but yes, they are currently the most flaky.
https://github.com/campersau/ungit/runs/639350614
campersau
force-pushed
the
puppeteer
branch
2 times, most recently
from
252406c
to
2fcb40d
Compare
|
First green build after a long time! |
Nighmare hasn't been updated for a while now and the chrome version is getting pretty old missing some features like
Promise.prototype.finally.Tests run fine on my machine but in CI they seem a bit flaky and need a bit more work. Might be because I removed a lot of
waits and puppeteer runs a lot faster and ungit sometimes can't keep up.It currently does not work in node 14 but that's because of this node issue which should be released in a few hours.Also using async / await!