Migrate clicktests from nightmare to puppeteer#1336
Migrate clicktests from nightmare to puppeteer#1336campersau merged 16 commits intoFredrikNoren:masterfrom
Conversation
| apt: | ||
| packages: | ||
| - xvfb | ||
| - libgbm1 |
There was a problem hiding this comment.
| 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.
| }, | ||
| 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.
| </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.
| 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?
| .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.
| }); | ||
| 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.
| } | ||
| 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! |
3 participants
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!