test: update power/notification specs to expect #13497
Conversation
spec/api-power-monitor-spec.js
Outdated
| after(async () => { | ||
| await reset() | ||
| }) | ||
| after(async () => { await reset() }) |
There was a problem hiding this comment.
Just pass reset to the after() call.
reset function returns a Promise, and after can properly handle it.
spec/api-notification-spec.js
Outdated
| assert.equal(n.actions[0].text, '3') | ||
| assert.equal(n.actions[1].type, 'button') | ||
| assert.equal(n.actions[1].text, '4') | ||
| expect(n.actions[0].type).to.equal('button') |
There was a problem hiding this comment.
Shouldn't we check that n.actions is an array of the expected size first?
spec/api-notification-spec.js
Outdated
| assert.equal(n.actions[0].text, '1') | ||
| assert.equal(n.actions[1].type, 'button') | ||
| assert.equal(n.actions[1].text, '2') | ||
| expect(n.actions[0].type).to.equal('button') |
There was a problem hiding this comment.
Shouldn't we check that n.actions is an array of the expected size first?
spec/api-power-monitor-spec.js
Outdated
| logindMock.on('MethodCalled', onceMethodCalled(done)) | ||
| // lazy load powerMonitor after we listen to MethodCalled mock signal | ||
| dbusMockPowerMonitor = require('electron').remote.powerMonitor | ||
| }) | ||
|
|
||
| it('should call Inhibit to delay suspend', async () => { | ||
| const calls = await getCalls() | ||
| assert.equal(calls.length, 1) | ||
| assert.deepEqual(calls[0].slice(1), [ | ||
| expect(calls.length).to.equal(1) |
spec/api-power-monitor-spec.js
Outdated
| dbusMockPowerMonitor.once('resume', () => done()) | ||
| emitSignal('org.freedesktop.login1.Manager', 'PrepareForSleep', | ||
| 'b', [['b', false]]) | ||
| }) | ||
|
|
||
| it('should have called Inhibit again', async () => { | ||
| const calls = await getCalls() | ||
| assert.equal(calls.length, 2) | ||
| assert.deepEqual(calls[1].slice(1), [ | ||
| expect(calls.length).to.equal(2) |
| assert.ok(idleState) | ||
| it('notify current system idle state', done => { | ||
| powerMonitor.querySystemIdleState(1, idleState => { | ||
| expect(idleState).to.be.true() | ||
| done() |
There was a problem hiding this comment.
I've heard that async/await is cool =)
it('notify current system idle state', async () => {
const idleState = await new Promise(
resolve => powerMonitor.querySystemIdleState(1, resolve))
expect(idleState).to.be.true()
})There was a problem hiding this comment.
It feels forced in this simple case, I would leave it as is
spec/api-power-save-blocker-spec.js
Outdated
| const id = powerSaveBlocker.start('prevent-app-suspension') | ||
| assert.ok(id != null) | ||
| assert.equal(powerSaveBlocker.isStarted(id), true) | ||
| expect(id).to.not.be.null() |
There was a problem hiding this comment.
If it's not null, what it is? A number, an object, a symbol?
|
@codebytere Sorry for a slow reaction to your PRs =/ |
|
@alexeykuzmin no worries 😁 i'm just coming back online after PTO so i'm gonna follow up and address these soon! |
miniak
force-pushed
the
expect-more-specs
branch
from
5bd9cc7
to
659cc92
Compare
|
@codebytere, @alexeykuzmin I've rebased the branch to fix the conflict with |
miniak
changed the title
miniak
force-pushed
the
expect-more-specs
branch
from
3e93255
to
b260387
Compare
alexeykuzmin
changed the title
There was a problem hiding this comment.
LGTM.
I agree with @alexeykuzmin about using async / await but it's not a blocker for an expectification PR
|
Only gn builds failed and only because the branch is relatively old. |
|
No Release Notes |
Description of Change
This PR updates the
notification,power-monitor, andpower-save-monitorspecs as part of ongoing work to migrate our specs fromassertto chai'sexpectwithdirty-chaifor better error messages./cc @alexeykuzmin
Checklist
npm testpassesRelease Notes
Notes: no-notes