From 1ee9d441804977f8c6c111c19deb69805518b563 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Wed, 27 Sep 2023 10:35:16 -0400 Subject: [PATCH 1/4] Regexy time simplification in task events --- ui/app/models/task-event.js | 21 +++++++++- ui/tests/unit/models/task-test.js | 65 +++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 1 deletion(-) diff --git a/ui/app/models/task-event.js b/ui/app/models/task-event.js index 680850885430..5a9de819c7c4 100644 --- a/ui/app/models/task-event.js +++ b/ui/app/models/task-event.js @@ -16,6 +16,25 @@ export default class TaskEvent extends Fragment { @attr('date') time; @attr('number') timeNanos; + @attr('string') displayMessage; - @attr('string') message; + get message() { + let message = simplifyTimeMessage(this.displayMessage); + return message; + } +} + +function simplifyTimeMessage(message) { + const match = message.match(/(\d+h)?(\d+m)?(\d+\.\d+)s/); + if (!match) return message; + + let [_, h, m, s] = match.map((x) => (x ? parseFloat(x) : 0)); + s = Math.round(s); + + m += Math.floor(s / 60); + s %= 60; + h += Math.floor(m / 60); + m %= 60; + + return `Task restarting in ${h ? h + 'h' : ''}${h || m ? m + 'm' : ''}${s}s`; } diff --git a/ui/tests/unit/models/task-test.js b/ui/tests/unit/models/task-test.js index 19073335592d..96e02e08ddfb 100644 --- a/ui/tests/unit/models/task-test.js +++ b/ui/tests/unit/models/task-test.js @@ -85,4 +85,69 @@ module('Unit | Model | task', function (hooks) { ); }); }); + + // Test that message comes back with proper time formatting + test('displayMessage shows simplified time', function (assert) { + assert.expect(5); + + const longTaskEvent = run(() => + this.owner.lookup('service:store').createRecord('task-event', { + displayMessage: 'Task restarting in 1h2m3.456s', + }) + ); + + assert.equal( + longTaskEvent.get('message'), + 'Task restarting in 1h2m3s', + 'hour-specific displayMessage is simplified' + ); + + const mediumTaskEvent = run(() => + this.owner.lookup('service:store').createRecord('task-event', { + displayMessage: 'Task restarting in 1m2.345s', + }) + ); + + assert.equal( + mediumTaskEvent.get('message'), + 'Task restarting in 1m2s', + 'minute-specific displayMessage is simplified' + ); + + const shortTaskEvent = run(() => + this.owner.lookup('service:store').createRecord('task-event', { + displayMessage: 'Task restarting in 1.234s', + }) + ); + + assert.equal( + shortTaskEvent.get('message'), + 'Task restarting in 1s', + 'second-specific displayMessage is simplified' + ); + + const roundedTaskEvent = run(() => + this.owner.lookup('service:store').createRecord('task-event', { + displayMessage: 'Task restarting in 1.999s', + }) + ); + + assert.equal( + roundedTaskEvent.get('message'), + 'Task restarting in 2s', + 'displayMessage is rounded' + ); + + const timelessTaskEvent = run(() => + this.owner.lookup('service:store').createRecord('task-event', { + displayMessage: 'All 3000 tasks look great, no notes.', + }) + ); + + assert.equal( + timelessTaskEvent.get('message'), + 'All 3000 tasks look great, no notes.', + 'displayMessage is unchanged' + ); + }); }); From 233728714f4a9f3593c696bfbe72e88d5f9cd82d Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Wed, 27 Sep 2023 10:40:23 -0400 Subject: [PATCH 2/4] Oops, dont assume these are all task restart messages --- ui/app/models/task-event.js | 24 ++++++++++++------------ ui/tests/unit/models/task-test.js | 4 ++-- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/ui/app/models/task-event.js b/ui/app/models/task-event.js index 5a9de819c7c4..c246407a8259 100644 --- a/ui/app/models/task-event.js +++ b/ui/app/models/task-event.js @@ -25,16 +25,16 @@ export default class TaskEvent extends Fragment { } function simplifyTimeMessage(message) { - const match = message.match(/(\d+h)?(\d+m)?(\d+\.\d+)s/); - if (!match) return message; - - let [_, h, m, s] = match.map((x) => (x ? parseFloat(x) : 0)); - s = Math.round(s); - - m += Math.floor(s / 60); - s %= 60; - h += Math.floor(m / 60); - m %= 60; - - return `Task restarting in ${h ? h + 'h' : ''}${h || m ? m + 'm' : ''}${s}s`; + return message.replace(/(\d+h)?(\d+m)?(\d+\.\d+)s/g, (_, h, m, s) => { + h = h ? parseInt(h) : 0; + m = m ? parseInt(m) : 0; + s = Math.round(parseFloat(s)); + + m += Math.floor(s / 60); + s %= 60; + h += Math.floor(m / 60); + m %= 60; + + return `${h ? h + 'h' : ''}${h || m ? m + 'm' : ''}${s}s`; + }); } diff --git a/ui/tests/unit/models/task-test.js b/ui/tests/unit/models/task-test.js index 96e02e08ddfb..20b432f3fb6c 100644 --- a/ui/tests/unit/models/task-test.js +++ b/ui/tests/unit/models/task-test.js @@ -128,13 +128,13 @@ module('Unit | Model | task', function (hooks) { const roundedTaskEvent = run(() => this.owner.lookup('service:store').createRecord('task-event', { - displayMessage: 'Task restarting in 1.999s', + displayMessage: 'I bet I can knock this out in about 1.999s', }) ); assert.equal( roundedTaskEvent.get('message'), - 'Task restarting in 2s', + 'I bet I can knock this out in about 2s', 'displayMessage is rounded' ); From 432b7650cfc4d5ca7bae4187f294df666f9476cc Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Wed, 27 Sep 2023 11:12:07 -0400 Subject: [PATCH 3/4] Update mirage to provide displayMessage instead of message --- ui/app/models/task-event.js | 26 +++++++++++--------- ui/mirage/factories/task-event.js | 2 +- ui/tests/acceptance/job-status-panel-test.js | 2 +- 3 files changed, 16 insertions(+), 14 deletions(-) diff --git a/ui/app/models/task-event.js b/ui/app/models/task-event.js index c246407a8259..a1c7e7b5ddf3 100644 --- a/ui/app/models/task-event.js +++ b/ui/app/models/task-event.js @@ -25,16 +25,18 @@ export default class TaskEvent extends Fragment { } function simplifyTimeMessage(message) { - return message.replace(/(\d+h)?(\d+m)?(\d+\.\d+)s/g, (_, h, m, s) => { - h = h ? parseInt(h) : 0; - m = m ? parseInt(m) : 0; - s = Math.round(parseFloat(s)); - - m += Math.floor(s / 60); - s %= 60; - h += Math.floor(m / 60); - m %= 60; - - return `${h ? h + 'h' : ''}${h || m ? m + 'm' : ''}${s}s`; - }); + return ( + message?.replace(/(\d+h)?(\d+m)?(\d+\.\d+)s/g, (_, h, m, s) => { + h = h ? parseInt(h) : 0; + m = m ? parseInt(m) : 0; + s = Math.round(parseFloat(s)); + + m += Math.floor(s / 60); + s %= 60; + h += Math.floor(m / 60); + m %= 60; + + return `${h ? h + 'h' : ''}${h || m ? m + 'm' : ''}${s}s`; + }) || '' + ); } diff --git a/ui/mirage/factories/task-event.js b/ui/mirage/factories/task-event.js index c9d2c84385e9..3e7fdfd0bfbc 100644 --- a/ui/mirage/factories/task-event.js +++ b/ui/mirage/factories/task-event.js @@ -17,5 +17,5 @@ export default Factory.extend({ exitCode: () => null, time: () => faker.date.past(2 / 365, REF_TIME) * 1000000, - message: () => faker.lorem.sentence(), + displayMessage: () => faker.lorem.sentence(), }); diff --git a/ui/tests/acceptance/job-status-panel-test.js b/ui/tests/acceptance/job-status-panel-test.js index 92bc220f027f..c14fc5674a45 100644 --- a/ui/tests/acceptance/job-status-panel-test.js +++ b/ui/tests/acceptance/job-status-panel-test.js @@ -790,7 +790,7 @@ module('Acceptance | job status panel', function (hooks) { await fillIn( '[data-test-history-search] input', - serverEvents.models[0].message + serverEvents.models[0].displayMessage ); assert.equal( findAll('.timeline-object').length, From 5daeec8d382d0546b129dd674125d9048e89c238 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Wed, 27 Sep 2023 12:25:54 -0400 Subject: [PATCH 4/4] Have a few acceptance tests look for .displayMessage instead of .message for equality now --- .changelog/18595.txt | 3 +++ ui/app/models/task-event.js | 2 +- ui/tests/acceptance/allocation-detail-test.js | 2 +- ui/tests/acceptance/task-detail-test.js | 2 +- 4 files changed, 6 insertions(+), 3 deletions(-) create mode 100644 .changelog/18595.txt diff --git a/.changelog/18595.txt b/.changelog/18595.txt new file mode 100644 index 000000000000..19ce7cab2a87 --- /dev/null +++ b/.changelog/18595.txt @@ -0,0 +1,3 @@ +```release-note:improvement +ui: simplify presentation of task event times (10m2.230948s bceomes 10m2s etc.) +``` diff --git a/ui/app/models/task-event.js b/ui/app/models/task-event.js index a1c7e7b5ddf3..56d879b8d46e 100644 --- a/ui/app/models/task-event.js +++ b/ui/app/models/task-event.js @@ -37,6 +37,6 @@ function simplifyTimeMessage(message) { m %= 60; return `${h ? h + 'h' : ''}${h || m ? m + 'm' : ''}${s}s`; - }) || '' + }) || message ); } diff --git a/ui/tests/acceptance/allocation-detail-test.js b/ui/tests/acceptance/allocation-detail-test.js index 5cd4a576a705..95a8f5e1ba08 100644 --- a/ui/tests/acceptance/allocation-detail-test.js +++ b/ui/tests/acceptance/allocation-detail-test.js @@ -209,7 +209,7 @@ module('Acceptance | allocation detail', function (hooks) { assert.equal(taskRow.name, task.name, 'Name'); assert.equal(taskRow.state, task.state, 'State'); - assert.equal(taskRow.message, event.message, 'Event Message'); + assert.equal(taskRow.message, event.displayMessage, 'Event Message'); assert.equal( taskRow.time, moment(event.time / 1000000).format("MMM DD, 'YY HH:mm:ss ZZ"), diff --git a/ui/tests/acceptance/task-detail-test.js b/ui/tests/acceptance/task-detail-test.js index dc73c21bd532..ff2c4c7a1429 100644 --- a/ui/tests/acceptance/task-detail-test.js +++ b/ui/tests/acceptance/task-detail-test.js @@ -223,7 +223,7 @@ module('Acceptance | task detail', function (hooks) { 'Event timestamp' ); assert.equal(recentEvent.type, event.type, 'Event type'); - assert.equal(recentEvent.message, event.message, 'Event message'); + assert.equal(recentEvent.message, event.displayMessage, 'Event message'); }); test('when the allocation is not found, the application errors', async function (assert) {