Details

Reviewers

anlambert
vlorentz

Group Reviewers

Reviewers

Maniphest Tasks

T3296: save code now detailed task window can display erratic creation date time

Commits

rDWAPPS2fb5a2e72feb: Fix malformatted date value when null

Summary

They are rendered as an epoch date instead of not being displayed. The
following bypasses the transformation if the value is null. In effect, such value should
no longer be displayed.

Depends on D5637

Related to T3296

Diff Detail

Repository

rDWAPPS Web applications

Branch

master

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 21057
Build 32680: Phabricator diff pipeline on jenkins	Jenkins console · Jenkins
Build 32679: arc lint + arc unit

Unit TestsFailed
View All

	Time	Test
	0 ms	Jenkins > should show not found for content::Tests / Cypress tests / Run cypress tests / Test origin-search Test invalid SWHIDs should show not found for content AssertionError: Timed out retrying after 20000ms: expected '<div.invalid-feedback>' to be 'visible' This element `<div.invalid-feedback>` is not visible because it has CSS property: `display: none`
	216 ms	Jenkins > .tox.py3.lib.python3.7.site-packages.swh.web.tests.admin.test_origin_save::Tests / Python tests / test_accept_pending_save_request
	828 ms	Jenkins > .tox.py3.lib.python3.7.site-packages.swh.web.tests.admin.test_origin_save::Tests / Python tests / test_add_authorized_origin_url
	123 ms	Jenkins > .tox.py3.lib.python3.7.site-packages.swh.web.tests.admin.test_origin_save::Tests / Python tests / test_add_unauthorized_origin_url
	130 ms	Jenkins > .tox.py3.lib.python3.7.site-packages.swh.web.tests.admin.test_origin_save::Tests / Python tests / test_reject_pending_save_request
		View Full Test Results (1 Failed · 645 Passed · 5 Skipped)

Event Timeline

ardumont created this revision.Apr 27 2021, 5:29 PM

ardumont added inline comments.Apr 27 2021, 5:37 PM

assets/src/bundles/save/index.js
348	may be simpler to read (plus it avoids one negation we so love for some reason)

Build has FAILED

Patch application report for D5624 (id=20064)

Rebasing onto 3d95c0667b...

Current branch diff-target is up to date.

Changes applied before test

commit 8789406eb0215fb968ae0875eee8e4eed2b3b447
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Apr 27 17:25:07 2021 +0200

    Fix malformatted date value when null
    
    They are rendered as an epoch date instead of not being displayed. The following bypass
    the transformation if the value is null. In effect, such value should no longer be
    displayed.
    
    Related to T3296

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/736/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/736/console

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 27 2021, 5:38 PM

Harbormaster failed remote builds in B21057: Diff 20064!

flaky test is flaky

return conditional and trigger the build again

Build is green

Patch application report for D5624 (id=20069)

Rebasing onto 3d95c0667b...

Current branch diff-target is up to date.

Changes applied before test

commit 55e47d6d7765c170f7e930dd072510a0504e4835
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Apr 27 17:25:07 2021 +0200

    Fix malformatted date value when null
    
    They are rendered as an epoch date instead of not being displayed. The following
    bypasses the transformation if the value is null. In effect, such value should no longer
    be displayed.
    
    Related to T3296

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/737/ for more details.

Harbormaster completed remote builds in B21062: Diff 20069.Apr 27 2021, 5:53 PM

ardumont requested review of this revision.Apr 27 2021, 5:53 PM

anlambert accepted this revision.Apr 27 2021, 5:59 PM

This revision is now accepted and ready to land.Apr 27 2021, 5:59 PM

Missing a test

This revision now requires changes to proceed.Apr 27 2021, 6:15 PM

oh come on now.

you tell me how to do it then.

@vlorentz Please, be advised that the strict non-leniency policy you start to
demonstrate in review is not a good one. Aside from frustrating people from doing the
right thing, (fixing the issue right now), I'm really not so sure it will serve us well
long-term.

Plus we do not have any coverage reporting tools which tells whether it's covered so...
The following could plainly be some smoke test for all I know (assuming i successfully
make it work).

In any case, I'll try my best with the following which might work, no idea yet:

describe('it should format appropriately value depending on their type', function() {
  for (type, value), expected_value in [
    // null values stays null
    (('json', null), null),
    (('date', null), null),
    (('raw', null), null),
    (('duration', null), null),
    // other values are formatted properly given their type
    (('json', '{}'), JSON.stringify('{}', null, 2)),
    (('date', '04/04/2021 00:00:00'), '4/4/2021 12:00:00AM'),
    (('raw', 'something-raw'), 'something-raw'),
    (('duration', '10'), '10 seconds'),
  ] {
    assert.equal(formatValuePerType(type, value), expected_value)
  }
})

almost one hour later, still trying to make that stuff comply:

describe('it should format appropriately values depending  on their type', function() {
  let input_values = [ // null values stay null
    {type: 'json', value: null, expected_value: null},
    {type: 'date', value: null, expected_value: null},
    {type: 'raw', value: null, expected_value: null},
    {type: 'duration', value: null, expected_value: null},
    // non null values formatted depending on their type
    {type: 'json', value: '{}', expected_value: JSON.stringify('{}', null, 2)},
    {type: 'date', value: '04/04/2021 00:00:00', expected_value: '4/4/2021 12:00:00AM'},
    {type: 'raw', value: 'value-for-identity', expected_value: 'value-for-identity'},
    {type: 'duration', value: '10', expected_value: '10 seconds'},
    {type: 'duration', value: 100, expected_value: '100 seconds'}
  ];
  input_values.forEach(function (input, index, array) {
    // console.log(`input=${input}`)
    assert.equal(formatValuePerType(input.type, input.value), input.expected_value)
  });
});

well, so far i cannot find a proper way to test it.

I don't seem to find an actionable document which explains to me how i can easily import
a function i want to test...

I'm kinda lost in js tools docs (mocha, cypress, ...) ¯\_(ツ)_/¯

Plus we do not have any coverage reporting tools which tells whether it's covered
so... The following could plainly be some smoke test for all I know (assuming i
successfully make it work).

I'm slightly wrong there. There is but it's not plugged in forge ui diff view (so not
seen here, the part i'm right about ;). As @anlambert reminded me at least twice, there
is coverage report in your local tree in cypress/coverage/lcov-report/index.html (I
gather it's there once you run the make test-frontend* targets).

In D5624#142717, @ardumont wrote:

almost one hour later, still trying to make that stuff comply:

describe('it should format appropriately values depending  on their type', function() {
  let input_values = [ // null values stay null
    {type: 'json', value: null, expected_value: null},
    {type: 'date', value: null, expected_value: null},
    {type: 'raw', value: null, expected_value: null},
    {type: 'duration', value: null, expected_value: null},
    // non null values formatted depending on their type
    {type: 'json', value: '{}', expected_value: JSON.stringify('{}', null, 2)},
    {type: 'date', value: '04/04/2021 00:00:00', expected_value: '4/4/2021 12:00:00AM'},
    {type: 'raw', value: 'value-for-identity', expected_value: 'value-for-identity'},
    {type: 'duration', value: '10', expected_value: '10 seconds'},
    {type: 'duration', value: 100, expected_value: '100 seconds'}
  ];
  input_values.forEach(function (input, index, array) {
    // console.log(`input=${input}`)
    assert.equal(formatValuePerType(input.type, input.value), input.expected_value)
  });
});

The following test to add in cypress/integration/origin-save.spec.js should work I think:

describe('it should format appropriately values depending  on their type', function() {
  let input_values = [ // null values stay null
    {type: 'json', value: null, expected_value: null},
    {type: 'date', value: null, expected_value: null},
    {type: 'raw', value: null, expected_value: null},
    {type: 'duration', value: null, expected_value: null},
    // non null values formatted depending on their type
    {type: 'json', value: '{}', expected_value: JSON.stringify('{}', null, 2)},
    {type: 'date', value: '04/04/2021 00:00:00', expected_value: '4/4/2021 12:00:00AM'},
    {type: 'raw', value: 'value-for-identity', expected_value: 'value-for-identity'},
    {type: 'duration', value: '10', expected_value: '10 seconds'},
    {type: 'duration', value: 100, expected_value: '100 seconds'}
  ];
  cy.window().then(win => {
    input_values.forEach(function (input, index, array) {
      assert.equal(win.swh.save.formatValuePerType(input.type, input.value), input.expected_value)
    });
  });
});

I'm slightly wrong there. There is but it's not plugged in forge ui diff view (so not
seen here, the part i'm right about ;). As @anlambert reminded me at least twice, there
is coverage report in your local tree in cypress/coverage/lcov-report/index.html (I
gather it's there once you run the make test-frontend* targets).

My bad, I should have added this info in the README.

Thanks a lot, will try (I was running low in energy ¯\_(ツ)_/¯ ;)

Add coverage

Finally it made it into the test \o/

vlorentz accepted this revision.Apr 28 2021, 11:37 AM

vlorentz added inline comments.

cypress/integration/origin-save.spec.js
113 ↗	(On Diff #20074)	c'mon

This revision is now accepted and ready to land.Apr 28 2021, 11:37 AM

Drop redundant js code and inline value instead.

Build has FAILED

Patch application report for D5624 (id=20074)

Rebasing onto 3d95c0667b...

Current branch diff-target is up to date.

Changes applied before test

commit de66613ef8c62392cfdb2833ea52a4638e30e058
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Apr 27 17:25:07 2021 +0200

    Fix malformatted date value when null
    
    They are rendered as an epoch date instead of not being displayed. The following
    bypasses the transformation if the value is null. In effect, such value should no longer
    be displayed.
    
    Related to T3296

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/739/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/739/console

Harbormaster failed remote builds in B21067: Diff 20074!Apr 28 2021, 11:41 AM

Build has FAILED

Patch application report for D5624 (id=20075)

Rebasing onto 3d95c0667b...

Current branch diff-target is up to date.

Changes applied before test

commit b1022e119b9455b1b03e8cb6206ff6b5b0157e81
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Apr 27 17:25:07 2021 +0200

    Fix malformatted date value when null
    
    They are rendered as an epoch date instead of not being displayed. The following
    bypasses the transformation if the value is null. In effect, such value should no longer
    be displayed.
    
    Related to T3296

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/740/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/740/console

Harbormaster failed remote builds in B21068: Diff 20075!Apr 28 2021, 11:49 AM

Build has FAILED

Patch application report for D5624 (id=20075)

Rebasing onto 3d95c0667b...

Current branch diff-target is up to date.

Changes applied before test

commit b1022e119b9455b1b03e8cb6206ff6b5b0157e81
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Apr 27 17:25:07 2021 +0200

    Fix malformatted date value when null
    
    They are rendered as an epoch date instead of not being displayed. The following
    bypasses the transformation if the value is null. In effect, such value should no longer
    be displayed.
    
    Related to T3296

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/741/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/741/console

build failed for unrelated reason (fix is in D5637)

Rebase on fixed master

Build is green

Patch application report for D5624 (id=20115)

Rebasing onto 50c6b1f01f...

Current branch diff-target is up to date.

Changes applied before test

commit 2fb5a2e72feb72f5a096c384989ba63babbd3f81
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Apr 27 17:25:07 2021 +0200

    Fix malformatted date value when null
    
    They are rendered as an epoch date instead of not being displayed. The following
    bypasses the transformation if the value is null. In effect, such value should no longer
    be displayed.
    
    Related to T3296

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/744/ for more details.

Harbormaster completed remote builds in B21107: Diff 20115.Apr 28 2021, 4:09 PM

Closed by commit rDWAPPS2fb5a2e72feb: Fix malformatted date value when null (authored by ardumont). · Explain WhyApr 28 2021, 4:10 PM

This revision was automatically updated to reflect the committed changes.

ardumont added a commit: rDWAPPS2fb5a2e72feb: Fix malformatted date value when null.

Fix malformatted date value when null
ClosedPublic
Actions

Details

Diff Detail

Unit TestsFailed
View All

Event Timeline

Patch application report for D5624 (id=20064)

Changes applied before test

Patch application report for D5624 (id=20069)

Changes applied before test

Patch application report for D5624 (id=20074)

Changes applied before test

Patch application report for D5624 (id=20075)

Changes applied before test

Patch application report for D5624 (id=20075)

Changes applied before test

Patch application report for D5624 (id=20115)

Changes applied before test

Revision Contents
Changeset List

Diff 20064

assets/src/bundles/save/index.js

Fix malformatted date value when nullClosedPublicActions

Details

Diff Detail

Unit TestsFailedView All

Event Timeline

Patch application report for D5624 (id=20064)

Changes applied before test

Patch application report for D5624 (id=20069)

Changes applied before test

Patch application report for D5624 (id=20074)

Changes applied before test

Patch application report for D5624 (id=20075)

Changes applied before test

Patch application report for D5624 (id=20075)

Changes applied before test

Patch application report for D5624 (id=20115)

Changes applied before test

Revision ContentsChangeset List

Diff 20064

assets/src/bundles/save/index.js

Fix malformatted date value when null
ClosedPublic
Actions

Unit TestsFailed
View All

Revision Contents
Changeset List