-
Notifications
You must be signed in to change notification settings - Fork 27
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
aotf: editable mutations #544
aotf: editable mutations #544
Conversation
Spotted two issues so far.
|
Fixed the broadcast issue by treating broadcast settings in a specially, requires cylc/cylc-flow#3986. |
fce5f38
to
d385057
Compare
Codecov Report
@@ Coverage Diff @@
## master #544 +/- ##
===========================================
- Coverage 82.80% 60.54% -22.27%
===========================================
Files 66 68 +2
Lines 1320 1366 +46
Branches 81 88 +7
===========================================
- Hits 1093 827 -266
- Misses 208 510 +302
- Partials 19 29 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
I've added some dummy mutations to the mocked workflow service so they can now be tested in offline mode and the first functional test, will test the mutation forms etc in due course. |
64f69b6
to
9ca0d45
Compare
@oliver-sanders I can write the tests for this one if you'd like? I just remembered how to use Cypress & write e2e tests working on #555 (slowly switching from vacation-mode to work-mode 😄). That way you can focus on other issues. |
That's a very kind offer, thanks. Just realised I hadn't committed on this PR since November 🤦. I had added some mock-mutations for the offline mode and had started writing tests for them but was running into issues running older Cypress on MacOS (had to quit and restart Cypress for every change which made the test development process painfully slow). |
Ah! So you should be able to finish the tests, hopefully without having to wait so much after #555 🤞 |
9ca0d45
to
090ab3f
Compare
... one month later 🤦 Rebased, got back to the tests... The good news is that Cypress has worked like a charm since the upgrade, seriously impressive project 👏! I've added functional tests to test that cylc-objects (cycle point, families, tasks, ...) are associated with mutations correctly. Now I'm having a go at capturing it('fires the mutation when clicked', () => {
cy.visit('/#/workflows/one')
const mutations = []
cy.window().its('app').then(app => {
// Object.getPrototypeOf(app.$workflowService).mutate = (stuff) => {
app.$workflowService.mutate = (stuff) => {
mutations.push(stuff) // add the mutation arguments to an array for later comparison
console.log('here')
}
})
// open the mutations panel
cy
.get('.node-data-cyclepoint > .c-task:first')
.should('exist')
.should('be.visible')
.click()
// run the first mutation in the list
cy
.get('.c-mutation-menu-list:first')
.find('.v-list-item__content:first')
.should('exist')
.should('be.visible')
.click()
.then(() => {
// ensure the mutation would have been executed
expect(mutations).to.deep.equal([])
})
}) No luck with that approach :( |
Does it work? cy.window().its('app.$workflowService').then(service => {
// Object.getPrototypeOf(app.$workflowService).mutate = (stuff) => {
cy.stub(service, 'mutate', () => {
mutations.push(stuff) // add the mutation arguments to an array for later comparison
console.log('here')
})
}) There's an example in the e2e test |
Ah, need to mock the |
Looks like you're almost done! It'll be great to have this PR merged. All mutations available from within the UI. I think you should be able to use |
090ab3f
to
0f1bcb5
Compare
Stub looks nice, thanks for the pointer, I think the Will come back to the unit tests tomorrow, hopefully get this finished soon 😓. |
4f320c3
to
58da4c5
Compare
* move labels from the fields to list item titles * add argument description in help tooltip * use enumation value description for the select hint * fix cycle point regex
58da4c5
to
cd42bea
Compare
Looks like CI is ok now! 🚀 |
cd42bea
to
7017c93
Compare
7017c93
to
66a3bc9
Compare
Sorted out the play/pause and stop buttons in the toolbar! The toolbar now sets an internal flag when it "mutates" a property and unsets that when the state changes. This property is then used to deactivate the buttons. Note:
|
Half-way through the code. Then review with a couple running workflows. But should finish reviewing it this morning or this afternoon 🎉 Looking great so far! |
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.
Few comments in the code, but no blockers. No need to update the code @oliver-sanders, mainly comments here and there. Tested UI + UIServer with couple workflows. Worked like a charm! Set outputs, release, hold, kill tasks. Also play/pause/stop workflow. 👏 and great tests!
I think for later we probably want a confirmation message before stopping workflows. WDYT @oliver-sanders @hjoliver ? I accidentally stopped my workflow during the tests 😄
src/components/graphqlFormGenerator/components/BroadcastSetting.vue
Outdated
Show resolved
Hide resolved
Thanks @kinow, good suggestions. Prompting on actions is something we need to look at, for example the In Cylc8 the "stop" button is now safe 🥳, you can always press "play" and everything will return to the way it was, so the need for a prompt on the stop button is much less than it was before. Trying to remember the Cylc7 GUI, I don't think we prompt for the stop button (but do for control => stop, which is equivalent to the new mutation editor). |
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.
LGTM, had a good play with it 👍
Addresses #339
Follow-on from: #504
Companion Cylc Flow PR: cylc/cylc-flow#3986 - merged
Example showing the trigger mutation being called, first using the configured defaults, then by edit.
Requirements check-list
CONTRIBUTING.md
and added my name as a Code Contributor.