Page MenuHomeSoftware Heritage

Fix malformatted date value when null
ClosedPublic

Authored by ardumont on Apr 27 2021, 5:29 PM.

Details

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 jenkinsJenkins console · Jenkins
Build 32679: arc lint + arc unit

Event Timeline

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!

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.

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

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).

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 ¯\_(ツ)_/¯ ;)

Finally it made it into the test \o/

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

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

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

ardumont edited the summary of this revision. (Show Details)EditedApr 28 2021, 3:33 PM

build failed for unrelated reason (fix is in D5637)

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.

This revision was automatically updated to reflect the committed changes.