-
Notifications
You must be signed in to change notification settings - Fork 297
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
[DDW-298] Add acceptance tests for update mechanism #977
[DDW-298] Add acceptance tests for update mechanism #977
Conversation
.then(() => daedalus.stores.NodeUpdateStore.refreshNextUpdate()) | ||
.then(done) | ||
.catch((error) => done(error)); | ||
}, { version: 50 }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MarcusHurney please extract this version value to a constant which we can use in later steps where you check if the version shown in the notification matches this expected one.
}); | ||
|
||
Then(/^I should see the correct version in the notification's title bar$/, async function () { | ||
const version = 50; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MarcusHurney this is actually expected version - please use the constant like described in https://github.com/input-output-hk/daedalus/pull/977/files#r195338177
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MarcusHurney great work!
There are just 2 minor details I would like to get solved before merging it!
@MarcusHurney looks like all tests after the one which applies the update fail (due to chrome unavailable). We need to fix that! |
…' of github.com:input-output-hk/daedalus into chore/ddw-298-add-acceptance-tests-for-update-mechanism Pulls in changes to CHANGELOG
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing those two issues @MarcusHurney - code now looks great!
We still need to see how to fix the failing tests though :(
…' of github.com:input-output-hk/daedalus into chore/ddw-298-add-acceptance-tests-for-update-mechanism [DDW-298] Pulls in latest changes
…ore/ddw-298-add-acceptance-tests-for-update-mechanism [DDW-298] Pulls in latest changes from develop branch
… node update scenario
…ccept node update
@nikolaglumac @DominikGuzei |
@MarcusHurney is this ready to be reviewed? |
@DominikGuzei AFAIK it should be ready. Please see last comment @MarcusHurney posted in this thread... |
features/support/electron.js
Outdated
// this ensures that the spectron instance of the app restarts | ||
// after the node update acceptance test shuts it down via 'kill-process' | ||
// eslint-disable-next-line prefer-arrow-callback | ||
After({ tags: '@preventAppQuit' }, async function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe @preventAppQuit
should be renamed to @restartApp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your request has been processed!
source/main/ipc-api/kill-process.js
Outdated
// if (event.sender !== mainWindow.webContents) return; | ||
// TODO: fix this | ||
process.exit(20); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MarcusHurney please be careful with this change - exit code 20 might be necessary for cardano-launcher … @cleverca22 can you take a look at this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DominikGuzei
Changing this line to app.quit ended up being unnecessary so I've changed it back. All tests still pass.
…ore/ddw-298-add-acceptance-tests-for-update-mechanism Merges latest from develop
@nikolaglumac from my side this PR is ready for merging! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great! 👍
This PR adds acceptance tests for the NodeUpdateNotification.js component's rendering behavior when a node update is made available. Two test scenarios include one where the node update is postponed and another when the update is accepted.
Todo:
Review Checklist:
Basics
npm run test
)npm run dev
)npm run package
/ CI builds)npm run flow:test
)npm run lint
)npm run manage:translations
produces no changes)npm run storybook
)package-lock.json
andyarn.lock
files are updatedCode Quality
Testing
After Review:
done
on the Youtrack board